Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix too many files being ignored when providing an ignore path containing directories #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jantzenw
Copy link

Problem: Providing an ignore path to "magento-deploy-ignore" containing directories causes other files in those directories to be ignored.

Set of steps that I used to reproduce the problem:

  1. Install Magento B2B module.
  2. Add these lines to "magento-deploy-ignore" in composer.json. For those curious, this is being done on a project because its composer installation removes the NegotiableQuote module (and several others, a la https://github.com/yireo/magento2-replace-tools) but the B2B base module has this Negotiable Quote fixture file. Its presence breaks setup:di:compile. This project does not use integration tests so this fixture file's absence will not be missed. This is of course one example to reproduce the problem. There are likely other better examples that do not have other solutions:
        "magento-deploy-ignore": {
            ...
            "magento/magento2-b2b-base": [
                "/setup/src/Magento/Setup/Fixtures/NegotiableQuotesFixture.php"
            ]
        },
  1. rm -rf setup vendor/magento/magento2-base/ vendor/magento/magento2-b2b-base && composer install
  2. See that other files are being ignored:
setup/src/Magento/Setup/Test/Unit/Fixtures/NegotiableQuotesFixtureTest.php
setup/src/Magento/Setup/Test/Unit/Fixtures/CompaniesFixtureTest.php
setup/src/Magento/Setup/Fixtures/_files/quote_fixture_data.json
setup/src/Magento/Setup/Fixtures/NegotiableQuotesFixture.php
setup/src/Magento/Setup/Fixtures/Quote/NegotiableQuoteConfiguration.php
setup/src/Magento/Setup/Fixtures/CompaniesFixture.php
setup/src/Magento/Setup/Fixtures/SharedCatalogsFixture.php
setup/performance-toolkit/files/1mb.pdf
setup/performance-toolkit/profiles/b2b
setup/performance-toolkit/profiles/b2b/extra_small.xml
setup/performance-toolkit/profiles/b2b/manufacture_standard.xml
setup/performance-toolkit/profiles/b2b/small.xml
setup/performance-toolkit/profiles/b2b/singlesite_small.xml
setup/performance-toolkit/profiles/b2b/medium.xml

This is because, in the strpos($destination,$ignored code, $destination is these values:

/setup/src/Magento
/setup/src/Magento/Setup
/setup/src/Magento/Setup/Test
/setup/src/Magento/Setup/Test/Unit
/setup/src/Magento/Setup/Test/Unit/Fixtures
/setup/src/Magento/Setup/Test/Unit/Fixtures/NegotiableQuotesFixtureTest.php
/setup/src/Magento/Setup/Test/Unit/Fixtures/CompaniesFixtureTest.php
/setup/src/Magento/Setup/Fixtures/_files
/setup/src/Magento/Setup/Fixtures/_files/quote_fixture_data.json
/setup/src/Magento/Setup/Fixtures/NegotiableQuotesFixture.php
/setup/src/Magento/Setup/Fixtures/Quote
/setup/src/Magento/Setup/Fixtures/Quote/NegotiableQuoteConfiguration.php
/setup/src/Magento/Setup/Fixtures/CompaniesFixture.php
/setup/src/Magento/Setup/Fixtures/SharedCatalogsFixture.php

strpos( '/setup/src/Magento/Setup/Fixtures/SharedCatalogsFixture.php', '/setup') returns 0, so the entire setup folder is ignored! This is because strpos takes the haystack as the first argument, not the second. So the arguments are reversed.

This seems like a significant change, so I hope that this will be well-reviewed. I am surprised that this bug exists but maybe this use case is rarer than I assumed, or this change will cause some other unintended effect or break some major use case that I am unaware of. The bug does go unnoticed for simple ignores not containing directories, e.g. "/.gitignore", "/.editorconfig", etc.

@jantzenw jantzenw closed this Apr 29, 2022
@jantzenw jantzenw reopened this Apr 29, 2022
@jantzenw
Copy link
Author

Trying to close and reopen pull request after signing Adobe CLA, per Adobe's instructions.

@jantzenw jantzenw closed this Apr 29, 2022
@jantzenw jantzenw reopened this Apr 29, 2022
@jantzenw jantzenw closed this Apr 29, 2022
@jantzenw jantzenw reopened this Apr 29, 2022
@convenient
Copy link

Cross linking with https://magento.stackexchange.com/a/350046/17176 just so I can share some notes more appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants