Setting number of address line lower than existing entry breaks all checkouts for existing address data

[ProxiBlue]

Preconditions and environment

• Mage-OS Magento version: 1.0.5 (likely all versions as this also exists in Magento 2.4.7)
• Clean install of mageos 1.0.5 with default sample data

Description:

If admin setting is changed to set number of allowed address lines lower than what is current config value, and data exists against current config value, checkout breaks complete. User is not allowed to checkout, and reason can be obscured, especially with 3rd party payment methods.

Steps to reproduce

1. Clean install of mageos 1.0.5 with default sample data
2. Set the address line value to 4.

./bin/magento con:set customer/address/street_lines 4

3. Access frontend, create a new user, and add a new billing/shipping address, using 4 lines fro address

4. proceed to add to cart any product, and checkout using any payment method.

Expected result: Checkout is a success.

5. Change the address allowed line lower. Any value below existing value will work.

./bin/magento con:set customer/address/street_lines 3

6. Now again goto checkout and try checkout.

address clearly still has 4 lines of data, and displays as such.

expected result: able to checkout

what actually happens:

Old address data (4 lines) block ability to checkout

NOTE:

This was using base cash on delivery payment method, but, if you for example use braintree / paypal, the error message is completely obscured.
The issue with braintree/paypal, is that the exception triggers a fail, which then tries to revert the order, but, since no order was actually place, a new exception occurs, overriding the address line one, and the user is not cleanly (if at all) told why they can't buy.

In both paypal/braintree usage, the error is simply a rest exception:

and the user is just stuck on a checkout page with a payment button that does not work.

I have noticed that the exception can be trapped by couponUsage plugin

which then for some reason ends up under orderCancelation, trying to rollback maybe?
This then fails with the fact there is no $incrementID, which raises a new exception, and obscures the real reason from client/frontend.

This can result in a lot of sales lost.

additionally:

Check address in my account

They display as before (is this fine ?) - showing 4 lines (does not break anything here)

Edit address:

The 4th address line is now dropped (but there is still data in db for it, as can be seen in previous display)

expected: Maybe the last lines should now be combined? (comma separated?)

Conclusion / expected / discussion

The address data is store in magento as a single one line field, no matter the number of address lines configured.
The 'lines' are separated by a 'newline' char.

IMO, the system should retain ability to go between address line numbers, and the change should definitely not affect checkout.
The code that results in the address lines, should populate the needed data accordingly.
If there are more delimiters ion the address line field, than the configured address lines, the last line should simply contain a concat of the remainder field values, with the newline stripped.

if you look at the data where the address is validated during checkout, it can clearly be seen that the resulting data is just splitting at newlines, and ignoring the configure address line limits.

line 156 on : vendor/mage-os/module-quote/Model/CustomerManagement.php

This should only be done for existing data, or maybe just during checkout.

Other outcomes/solution suggestions?

Expected result

I can checkout irrespective if my address data was more lines that last time I checked out :)

Actual result

User is stuck / cannot checkout / reason is not clear.

Additional information

No response

Release note

No response

Triage and priority

• Severity: S0 - Affects critical data or functionality and leaves users without workaround.
• Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
• Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
• Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
• Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

[ProxiBlue]

Digging into the code, it looks like thi is where it is going wrong:

Magento\Customer\Model\Address\AbstractAddress::getDataModel

[ProxiBlue]

which then ends up here:

which is exactly the issue.

Checkout assumes / expects data to conform to the max lines, and just splits it.
However if that setting is changed, then there is no data update, and there should not be, a that could kill a db with a long lock if there is a crap load of customers.

So, the fix will be to not assume the max characters, but actually produce the result here, base don max characters, and IMO any 'spill over' rows be concatenated to the last line.

So, if we have set 1 row, then they will all conact to 1 row, and if we set back to 3 rows, data will be able to transform.
The issue will only be if we started with 1 line, and then make it 4 lines, all will rmain in one line, BUT, that is ok, and expected, and will not kill checkout.

[ProxiBlue]

Ok, here is a patch for the fix, which can be used im composer patches to fix this in an urgency.

Proper MR pending, as I want to see about adding some tests to check for this going forward, but need to get client site fixed first :)

This code fixed the lowest level call to 'abstract' address class (not abstract by class def, just by name)

public function getStreet()
    {
        if (is_array($this->getStreetFull())) {
            return $this->getStreetFull();
        }
        $maxAllowedLineCount = $this->addressHelper->getStreetLines() ?? 2;
        $lines = explode("\n", $this->getStreetFull());
        $extraLines = array_slice($lines, $maxAllowedLineCount);
        $lines = array_slice($lines, 0, $maxAllowedLineCount);
        if (!empty($extraLines)) {
            $lines[$maxAllowedLineCount - 1] .= ', ' . implode(', ', $extraLines);
        }
        $this->setStreetFull(implode("\n", $lines));
        return $lines;
    }

This then fixes the display everywhere.

this looks like 4 lines, but is 3, so, just looks 'normal'

I tested setting to 1 line:

underlying data in address object now conforms to address limits set in admin

Then swapped to 1 line

```./bin/magento con:set customer/address/street_lines 1``

the raw underlaying data is still intact to number of lines set when that address was last saved.

[ProxiBlue]

ok, won't let me upload as attachement

This is for 1.0.x / magento 2.4.7

diff --git a/Model/Address/AbstractAddress.php b/Model/Address/AbstractAddress.php
index 75474ba..0cee601 100644
--- a/Model/Address/AbstractAddress.php
+++ b/Model/Address/AbstractAddress.php
@@ -17,6 +17,7 @@ use Magento\Customer\Model\Data\Address as AddressData;
 use Magento\Framework\App\ObjectManager;
 use Magento\Framework\Model\AbstractExtensibleModel;
 use Magento\Framework\ObjectManager\ResetAfterRequestInterface;
+use Magento\Customer\Helper\Address as AddressHelper;

 /**
  * Address abstract model
@@ -146,6 +147,11 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
      */
     private array $regionIdCountry = [];

+    /**
+     * @var AddressHelper|null
+     */
+    protected ?AddressHelper $addressHelper;
+
     /**
      * @param \Magento\Framework\Model\Context $context
      * @param \Magento\Framework\Registry $registry
@@ -166,6 +172,7 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
      * @param CompositeValidator $compositeValidator
      * @param CountryModelsCache|null $countryModelsCache
      * @param RegionModelsCache|null $regionModelsCache
+     * @param Magento\Customer\Helper\Address\AddressHelper|null $addressHelper
      *
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      */
@@ -189,6 +196,7 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
         CompositeValidator $compositeValidator = null,
         ?CountryModelsCache $countryModelsCache = null,
         ?RegionModelsCache $regionModelsCache = null,
+        ?AddressHelper $addressHelper = null
     ) {
         $this->_directoryData = $directoryData;
         $data = $this->_implodeArrayField($data);
@@ -206,6 +214,8 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
             ->get(CountryModelsCache::class);
         $this->regionModelsCache = $regionModelsCache ?: ObjectManager::getInstance()
             ->get(RegionModelsCache::class);
+        $this->addressHelper = $addressHelper ?: ObjectManager::getInstance()
+            ->get(AddressHelper::class);
         parent::__construct(
             $context,
             $registry,
@@ -243,6 +253,8 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
     /**
      * Retrieve street field of an address
      *
+     * honour street line limits as set in admin
+     *
      * @return string[]
      */
     public function getStreet()
@@ -250,7 +262,15 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
         if (is_array($this->getStreetFull())) {
             return $this->getStreetFull();
         }
-        return explode("\n", $this->getStreetFull());
+        $maxAllowedLineCount = $this->addressHelper->getStreetLines() ?? 2;
+        $lines = explode("\n", $this->getStreetFull());
+        $extraLines = array_slice($lines, $maxAllowedLineCount);
+        $lines = array_slice($lines, 0, $maxAllowedLineCount);
+        if (!empty($extraLines)) {
+            $lines[$maxAllowedLineCount - 1] .= ', ' . implode(', ', $extraLines);
+        }
+        $this->setStreetFull(implode("\n", $lines));
+        return $lines;
     }

     /**
diff --git a/etc/di.xml b/etc/di.xml
index 04dee8d..3f863be 100644
--- a/etc/di.xml
+++ b/etc/di.xml
@@ -593,4 +593,9 @@
                 type="Magento\Customer\Plugin\AsyncRequestCustomerGroupAuthorization"
         />
     </type>
+    <type name="Magento\Customer\Model\Address\AbstractAddress">
+        <arguments>
+            <argument name="addressHelper" xsi:type="object">Magento\Customer\Helper\Address</argument>
+        </arguments>
+    </type>
 </config>

[ProxiBlue]

Here is a magento 2.4.4-p11 patch.

diff --git a/Model/Address/AbstractAddress.php b/Model/Address/AbstractAddress.php
index bdf29d6e..5dd9a4c8 100644
--- a/Model/Address/AbstractAddress.php
+++ b/Model/Address/AbstractAddress.php
@@ -14,6 +14,7 @@ use Magento\Customer\Api\Data\RegionInterfaceFactory;
 use Magento\Customer\Model\Data\Address as AddressData;
 use Magento\Framework\App\ObjectManager;
 use Magento\Framework\Model\AbstractExtensibleModel;
+use Magento\Customer\Helper\Address as AddressHelper;

 /**
  * Address abstract model
@@ -120,6 +121,11 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
     /** @var CompositeValidator */
     private $compositeValidator;

+    /**
+     * @var AddressHelper|null
+     */
+    protected AddressHelper $addressHelper;
+
     /**
      * @param \Magento\Framework\Model\Context $context
      * @param \Magento\Framework\Registry $registry
@@ -138,6 +144,7 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
      * @param \Magento\Framework\Data\Collection\AbstractDb $resourceCollection
      * @param array $data
      * @param CompositeValidator $compositeValidator
+     * @param Magento\Customer\Helper\Address\AddressHelper|null $addressHelper
      *
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      */
@@ -158,7 +165,8 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
         \Magento\Framework\Model\ResourceModel\AbstractResource $resource = null,
         \Magento\Framework\Data\Collection\AbstractDb $resourceCollection = null,
         array $data = [],
-        CompositeValidator $compositeValidator = null
+        CompositeValidator $compositeValidator = null,
+        AddressHelper $addressHelper = null
     ) {
         $this->_directoryData = $directoryData;
         $data = $this->_implodeArrayField($data);
@@ -172,6 +180,8 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
         $this->dataObjectHelper = $dataObjectHelper;
         $this->compositeValidator = $compositeValidator ?: ObjectManager::getInstance()
             ->get(CompositeValidator::class);
+        $this->addressHelper = $addressHelper ?: ObjectManager::getInstance()
+            ->get(AddressHelper::class);
         parent::__construct(
             $context,
             $registry,
@@ -216,7 +226,15 @@ class AbstractAddress extends AbstractExtensibleModel implements AddressModelInt
         if (is_array($this->getStreetFull())) {
             return $this->getStreetFull();
         }
-        return explode("\n", $this->getStreetFull());
+        $maxAllowedLineCount = $this->addressHelper->getStreetLines() ?? 2;
+        $lines = explode("\n", $this->getStreetFull());
+        $extraLines = array_slice($lines, $maxAllowedLineCount);
+        $lines = array_slice($lines, 0, $maxAllowedLineCount);
+        if (!empty($extraLines)) {
+            $lines[$maxAllowedLineCount - 1] .= ', ' . implode(', ', $extraLines);
+        }
+        $this->setStreetFull(implode("\n", $lines));
+        return $lines;
     }

     /**
diff --git a/etc/di.xml b/etc/di.xml
index 156986b7..6c2d2f2b 100644
--- a/etc/di.xml
+++ b/etc/di.xml
@@ -560,4 +560,9 @@
         <plugin name="getByIdCustomerGroupExcludedWebsite" type="Magento\Customer\Model\Plugin\GetByIdCustomerGroupExcludedWebsite"/>
         <plugin name="getListCustomerGroupExcludedWebsite" type="Magento\Customer\Model\Plugin\GetListCustomerGroupExcludedWebsite"/>
     </type>
+    <type name="Magento\Customer\Model\Address\AbstractAddress">
+        <arguments>
+            <argument name="addressHelper" xsi:type="object">Magento\Customer\Helper\Address</argument>
+        </arguments>
+    </type>
 </config>

[ProxiBlue]

Damn, i was working on the tests, and then found this hidden in the helper

public function convertStreetLines($origStreets, $toCount)
    {
        $lines = [];
        if (!empty($origStreets) && $toCount > 0) {
            $countArgs = (int)floor(count($origStreets) / $toCount);
            $modulo = count($origStreets) % $toCount;
            $offset = 0;
            $neededLinesCount = 0;
            for ($i = 0; $i < $toCount; $i++) {
                $offset += $neededLinesCount;
                $neededLinesCount = $countArgs;
                if ($modulo > 0) {
                    ++$neededLinesCount;
                    --$modulo;
                }
                $values = array_slice($origStreets, $offset, $neededLinesCount);
                if (is_array($values)) {
                    $lines[] = implode(' ', $values);
                }
            }
        }

        return $lines;
    }

so, rather than having my own code adjusted like I did, I am refactoring to use this, so as to conform to anywhere else doing this.

[ProxiBlue]

public function getStreet()
    {
        if (is_array($this->getStreetFull())) {
            return $this->getStreetFull();
        }
        $maxAllowedLineCount = $this->addressHelper->getStreetLines() ?? 2;
        $lines = explode("\n", $this->getStreetFull());
        $lines = $this->addressHelper->convertStreetLines($lines, $maxAllowedLineCount);
        $this->setStreetFull(implode("\n", $lines));
        return $lines;
    }

yes, using the core helper method works just as well, they simply did it the other way round, concatenating into teh first line, not the last, as I did.

This introduces less new core code, so will be how I will do the MR