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

Update registration files formatting #2204

Merged

Conversation

davidalger
Copy link
Member

@davidalger davidalger commented Oct 27, 2015

Adding a simple use statement to tighten up the code :)

@orlangur
Copy link
Contributor

There is a failure in integration tests after these changes:
https://travis-ci.org/magento/magento2/jobs/87774509

There was 1 failure:
1) Magento\Setup\Console\Command\DependenciesShowFrameworkCommandTest::testExecute
Failed asserting that '"Dependencies of framework:","Total number"
,2
"Dependencies for each module:",
"Magento\A",1
" -- Magento\Framework",2
"Magento\B",1
" -- Magento\Framework",2
' contains ""Magento\A",1
" -- Magento\Framework",3
".
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Setup/Console/Command/DependenciesShowFrameworkCommandTest.php:77

@davidalger
Copy link
Member Author

@orlangur I'll run the test locally to see what's setting it off and post back when I figure it out.

@davidalger davidalger force-pushed the feature/use-namespaces-in-registration branch from 92c6935 to bbede11 Compare October 28, 2015 16:27
@davidalger
Copy link
Member Author

@orlangur This should be taken care of in bbede1146. The issue was DependenciesShowFrameworkCommandTest relied on an assumption that there were two references to Magento\Framework in the mock modules' registration.php files. Since I removed one of them by introducing the use directive, the test was operating under an incorrect assumption. I updated the test to make the correct assertion and it ran successfully for me on local.

@orlangur
Copy link
Contributor

Good 👍 I supposed it was a fragile test rather than a new problem introduced by changes.

David Alger added 2 commits November 10, 2015 13:12
The `DependenciesShowFrameworkCommandTest` implementation relied on an assumption that there were two references to `Magento\Framework` in the mock modules. Since the change to the registration.php files reduced this to one, the total dependency count dropped from 3 to 2.
@davidalger davidalger force-pushed the feature/use-namespaces-in-registration branch from bbede11 to 1d514f7 Compare November 10, 2015 19:12
@dsikkema-magento
Copy link
Contributor

Internal ticket created: MAGETWO-46377

@vancoz vancoz added question and removed question labels Jan 11, 2016
@sshrewz sshrewz added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Feb 22, 2016
@dsikkema-magento
Copy link
Contributor

Thank you for your willingness to contribute, but I don't think we'll accept this right now, for the simple reason that it would be more consistent to make the corresponding changes to dozens of other modules in other packages (such as EE modules), and only internal developers have access to that.

@davidalger
Copy link
Member Author

davidalger commented Jun 5, 2017

@vrann / @ishakhsuvarov

Any chance this might be re-looked at if I took the time to resolve the conflicts that have cropped up since it was closed in 2016?

@orlangur
Copy link
Contributor

orlangur commented Jun 6, 2017

@davidalger could you provide an automated way to reproduce such changes?

I don't think it is reasonable to resolve conflicts, we just need an automated way to do it quickly in ce/ee/b2b.

Generally, I'm going to replace all FQCN with aliases, which will cover those registration.php files as well, do you still think we should merge this change separately?

@davidalger
Copy link
Member Author

@orlangur I will see what I can do.

@ishakhsuvarov
Copy link
Contributor

Hi @davidalger
Any progress on this? Do you need any assistance?

@okorshenko
Copy link
Contributor

Hi @davidalger Do you need some help?

@davidalger
Copy link
Member Author

davidalger commented Jul 7, 2017

@okorshenko / @ishakhsuvarov

Sorry it took me so long to get back to this. I've updated the branch and applied the change to all the new registration.php files.

Here is how I did it:

find . -type f -name registration.php | xargs -n 1 \
    perl -0777 -pi -e 's#(.*)::register\(\n.*(ComponentRegistrar::.*),\n\s+(.*),\n\s+(.*)\n#\nuse $1;\n\nComponentRegistrar::register($2, $3, $4#'

find . -type f -name registration.php | xargs -n 1 php -l | grep -v 'No syntax errors detected'

git status -s

@okorshenko okorshenko self-assigned this Jul 10, 2017
@okorshenko okorshenko added this to the July 2017 milestone Jul 10, 2017
@magento-team magento-team added Progress: accept Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development develop labels Jul 10, 2017
@magento-team magento-team merged commit 7036bec into magento:develop Jul 10, 2017
@davidalger davidalger deleted the feature/use-namespaces-in-registration branch September 27, 2018 15:33
@Ctucker9233
Copy link

@magento-team I don't see this PR in 2.2 as of 2.2.7. Should it be ported?

@orlangur
Copy link
Contributor

@Ctucker9233 no, it shouldn't, and it cannot be actually according to current rules as it is just some cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants