21842: don't cache absolute file paths in validator factory #21856

david-fuehr · 2019-03-20T12:39:24Z

Description

This issue comes from Magento\Framework\Validator\Factory::_initializeConfigList where absolute file paths are cached. The actual file content is evaluated using the local file system.

The method is used in \Magento\Customer\Model\ResourceModel\Address::_validate, \Magento\Quote\Model\CustomerManagement::validateAddresses and \Magento\Customer\Model\ResourceModel\Customer::_validate and thus an error (The "/<abs_instance_path>/app/code/Magento/Eav/etc/validation.xml" file doesn't exist.) occurs whenever a customer, a customer_address or quote for a registered customer is saved.

Fixed Issues

Checkout error for registered customer with cache_id_prefix on multi server setup #21842: Checkout error for registered customer with cache_id_prefix on multi server setup

Manual testing scenarios

most realistic scenario

have a multi server setup with
1. node backend (absolute instance dir: /var/www/backend/release/2019-02-28)
2. node frontend-a (absolute instance dir: /var/www/frontend/release/2019-02-28)
3. node frontend-b (absolute instance dir: /var/www/frontend/release/2019-03-01 - autospawned)
4. node redis
5. share cache for all Magento instances (identical id_prefix in cache section of env.php)
6. [node mysql, ...]
clear cache on backend node
right after that save a customer or customer_address on the backend node (may be a cron job or similar)
place an order with a registered customer on one of the frontend-nodes
1.expected: The success page is being shown

simplified repro steps

set up a regular instance
make sure that the id_prefix in env.php is defined

    'cache' => [
        'frontend' => [
            'default' => [
                'id_prefix' => '173_'
            ],
            'page_cache' => [
                'id_prefix' => '173_'
            ]
        ]
    ],

redis may be used, the issue can also be reproduced using file based caching
register a customer
place an order using this customer
move the instance directory to another path
update webserver configuration accordingly and restart
try to place an order with the same customer again
1. expected: success page is being shown

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

…velop

Github issue: magento#21842 In customer and customer_address validation context the validator factory no longer caches absolute file paths for the validation.xml files (currently two file) as the file content is evaluated later in the filesystem directly and the file paths may not be correct in a multi server setup with shared cache (id_prefix)

- adapt unit test

magento-engcom-team · 2019-03-20T12:39:27Z

Hi @david-fuehr. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

- refactoring of static method use should be in a separate pull request

magento-engcom-team · 2019-03-20T20:55:37Z

Hi @larsroettig, thank you for the review.
ENGCOM-4545 has been created to process this Pull Request

sidolov · 2019-03-29T15:16:07Z

lib/internal/Magento/Framework/Validator/Factory.php

 class Factory
 {
-    /** cache key */
-    const CACHE_KEY = __CLASS__;


Unfortunately, we are not allowed to remove the constants according to our Backward Compatible Development Guide. Just mark it as deprecated

Thanks for your feedback. I addressed this in my latest commit.

sidolov · 2019-03-29T15:16:58Z

lib/internal/Magento/Framework/Validator/Factory.php

     */
-    protected $_configFiles = null;
+    protected $_configFiles;


We can't change the default value of public/protected properties according to our Backward Compatible Development Guide. Just mark it as deprecated

Thanks for your feedback. I addressed this in my latest commit.

sidolov · 2019-03-29T15:17:48Z

lib/internal/Magento/Framework/Validator/Factory.php

     */
    public function __construct(
-        \Magento\Framework\ObjectManagerInterface $objectManager,
-        \Magento\Framework\Module\Dir\Reader $moduleReader,
-        FrontendInterface $cache


Please, leave FrontendInterface as constructor parameter and mark it as deprecated, otherwise it's backward incompatible change

Thanks for your feedback. I addressed this in my latest commit.

magento-engcom-team · 2019-04-01T10:01:37Z

Hi @sivaschenko, thank you for the review.
ENGCOM-4545 has been created to process this Pull Request

VasylShvorak · 2019-04-17T11:43:02Z

✔️ QA passed

…dont-cache-absolute-file-paths

m2-assistant · 2019-04-29T14:06:53Z

Hi @david-fuehr, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

…ory #21856

david-fuehr added 4 commits March 14, 2019 21:30

Merge branch '2.3-develop' of github.com:magento/magento2 into 2.3-de…

1106542

…velop

Merge branch '2.3-develop' of github.com:magento/magento2 into 2.3-de…

eb989e2

…velop

clean up Validator Factory

e266d9a

- adapt unit test

magento-engcom-team added Component: Validator Release Line: 2.3 Partner: TechDivision partners-contribution Pull Request is created by Magento Partner labels Mar 20, 2019

untouch static method call to prevent codacy fail

83b34b8

- refactoring of static method use should be in a separate pull request

larsroettig approved these changes Mar 20, 2019

View reviewed changes

ghost assigned larsroettig Mar 20, 2019

ghost added the Progress: ready for testing label Mar 20, 2019

larsroettig requested a review from melnikovi March 20, 2019 20:57

sidolov requested changes Mar 29, 2019

View reviewed changes

ghost assigned sidolov Mar 29, 2019

ghost added Progress: needs update and removed Progress: ready for testing labels Mar 29, 2019

larsroettig self-requested a review March 29, 2019 15:35

changes due to backword compatiblity constraints

f3d4d96

sivaschenko approved these changes Apr 1, 2019

View reviewed changes

ghost assigned sivaschenko Apr 1, 2019

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 1, 2019

VasylShvorak self-assigned this Apr 17, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Apr 17, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Apr 17, 2019

p-bystritsky self-assigned this Apr 18, 2019

p-bystritsky added 2 commits April 18, 2019 15:17

Merge remote-tracking branch 'mainline/2.3-develop' into issue-21842-…

b77d8d0

…dont-cache-absolute-file-paths

magento#21856: Static test fix.

875fa4c

ghost added Progress: accept and removed Progress: extended testing labels Apr 19, 2019

magento-engcom-team mentioned this pull request Apr 29, 2019

Checkout error for registered customer with cache_id_prefix on multi server setup #21842

Closed

magento-engcom-team merged commit 875fa4c into magento:2.3-develop Apr 29, 2019

magento-engcom-team added this to the Release: 2.3.2 milestone Apr 29, 2019

magento-engcom-team pushed a commit that referenced this pull request Apr 29, 2019

ENGCOM-4545: 21842: don't cache absolute file paths in validator fact…

c65ead5

…ory #21856

david-fuehr mentioned this pull request May 7, 2019

[Backport] 21842: don't cache absolute file paths in validator factory david-fuehr/magento2#1

Open

4 tasks

david-fuehr pushed a commit to david-fuehr/magento2 that referenced this pull request May 7, 2019

magento#21856: Static test fix.

6fcf88a

david-fuehr pushed a commit to david-fuehr/magento2 that referenced this pull request May 9, 2019

magento#21856: Static test fix.

ac0559e

david-fuehr mentioned this pull request May 9, 2019

[Backport] 21842: don't cache absolute file paths in validator factory #22805

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

21842: don't cache absolute file paths in validator factory #21856

21842: don't cache absolute file paths in validator factory #21856

david-fuehr commented Mar 20, 2019 •

edited

Loading

magento-engcom-team commented Mar 20, 2019

magento-engcom-team commented Mar 20, 2019

sidolov Mar 29, 2019

david-fuehr Apr 1, 2019

sidolov Mar 29, 2019

david-fuehr Apr 1, 2019

sidolov Mar 29, 2019

david-fuehr Apr 1, 2019

magento-engcom-team commented Apr 1, 2019

VasylShvorak commented Apr 17, 2019

m2-assistant bot commented Apr 29, 2019

21842: don't cache absolute file paths in validator factory #21856

21842: don't cache absolute file paths in validator factory #21856

Conversation

david-fuehr commented Mar 20, 2019 • edited Loading

Description

Fixed Issues

Manual testing scenarios

most realistic scenario

simplified repro steps

Contribution checklist

magento-engcom-team commented Mar 20, 2019

magento-engcom-team commented Mar 20, 2019

sidolov Mar 29, 2019

Choose a reason for hiding this comment

david-fuehr Apr 1, 2019

Choose a reason for hiding this comment

sidolov Mar 29, 2019

Choose a reason for hiding this comment

david-fuehr Apr 1, 2019

Choose a reason for hiding this comment

sidolov Mar 29, 2019

Choose a reason for hiding this comment

david-fuehr Apr 1, 2019

Choose a reason for hiding this comment

magento-engcom-team commented Apr 1, 2019

VasylShvorak commented Apr 17, 2019

m2-assistant bot commented Apr 29, 2019

david-fuehr commented Mar 20, 2019 •

edited

Loading