Solve bug #116 - Setting number of address line lower breaks checkout #117
Conversation
…sting entry breaks all checkouts for existing address data
|
I specifically coded not to do that, as those child classes can be extended by 3rd party..
My concern is that this can break 3rd party modules.
If that is not a concern I will happily adjust all child classes.
…---- On Fri, 24 Jan 2025 22:23:35 +0800 ***@***.*** wrote ----
@rhoerr requested changes on this pull request.
Please also add the new constructor argument to the child classes of AbstractAddress, \Magento\Customer\Model\Address and \Magento\Quote\Model\Quote\Address.
Otherwise, looks good!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.
|
It should be fully intercompatible, as long as you make the new argument optional like you did for AbstractAddress: If a child class or preference of CustomerAddress or QuoteAddress doesn't define addressHelper, the optional param and ObjectManager fallback will mean it just gets created at runtime anyway -- no errors. The reason I ask for this is otherwise there's no way to touch or modify $addressHelper by DI or child class (not that there would be need to, but we should be consistent about it). The major downside of adding it to the child classes is a very high chance of future merge conflicts, unless this gets merged into Magento too (hopefully it does). I'm confident they'll ask for the same though. Thank you |
|
Gotcha, I was concerned to not break other stuff, but your eplanation is sound. Same thing, just in two lower classes, will adjust
…---- On Fri, 24 Jan 2025 22:41:55 +0800 Ryan Hoerr ***@***.***> wrote ---
I specifically coded not to do that, as those child classes can be extended by 3rd party.. My concern is that this can break 3rd party modules. If that is not a concern I will happily adjust all child classes.
It should be fully intercompatible, as long as you make the new argument optional like you did for AbstractAddress:
?AddressHelper $addressHelper = null
...
$this->addressHelper = $addressHelper ?: ObjectManager::getInstance()
->get(AddressHelper::class);
If a child class or preference of CustomerAddress or QuoteAddress doesn't define addressHelper, the optional param and ObjectManager fallback will mean it just gets created at runtime anyway -- no errors.
The reason I ask for this is otherwise there's no way to touch or modify $addressHelper by DI or child class (not that there would be need to, but we should be consistent about it).
The major downside of adding it to the child classes is a very high chance of future merge conflicts, unless this gets merged into Magento too (hopefully it does). I'm confident they'll ask for the same though.
Thank you
—
Reply to this email directly, #117 (comment), or https://github.com/notifications/unsubscribe-auth/ABGDJVBBPLV63LGPWQH5WHT2MJGLHAVCNFSM6AAAAABVPM6RXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJSGY4TGNZRGQ.
You are receiving this because you authored the thread.
|
|
@rhoerr adjustments made as requested. |
|
@ProxiBlue @rhoerr this PR targets 2.4-develop, shouldn't it be on release/1.x? |
|
We can cherry pick it (probably in a new PR) once it's merged. |
|
but should it even be on 2.4-develop in the first place? |
|
It's fine to be there -- ultimately our release/1.x branch is going to be remerged from 2.4-develop once Magento 2.4.8 comes out. We are tracking them separately because 2.4-develop has unstable code from upstream right now, but we have customizations on 2.4-develop as well (it's not purely a copy of upstream). |
I did actually query this in magechat :)
It was not clear / i missed it, where to fork on / develop on, as all instructions pointed to adobe/-develop (and that DID feel wrong tbh) |
|
@rhoerr once we merge it should I creare a PR for the cherry-pick to release/1.x or we'll do a full merge before the next release? |
Description (*)
Refer to bug #116
The code adjusts the fetch of Street lines to ensure result matches expected address lines, if legacy data.
Legacy data with address lines > as current configured will break checkout, as described in detail in bug.
The code uses existing method in core that is meant to do this, but was never actually called.
Related Pull Requests
N/A
Fixed Issues (if relevant)
Manual testing scenarios (*)
The issue is directly related to legacy address data in address fields, after address was limited to lesser address lines.
The first step is to get a longer address saved, and used, then go back, and retry after address lines were changed.
Extensive detailed testing in note din the ticket on how to reproduce. Refer to bug #116
Contribution checklist (*)