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

[2.2-dev] Move functions.php into Framework #16800

Merged
merged 1 commit into from Sep 6, 2018

Conversation

@fooman
Copy link
Contributor

commented Jul 14, 2018

See discussion and comments in #16705 - this is a pull request for 2.2

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)
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2018

Hi @fooman. 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 {$VERSION} instance - deploy vanilla Magento instance

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

@fooman fooman referenced this pull request Jul 14, 2018
0 of 4 tasks complete

@orlangur orlangur self-assigned this Jul 15, 2018

@fooman fooman force-pushed the fooman:move-functions-2.2 branch from 338a7c0 to 4fcb412 Jul 18, 2018

@fooman

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2018

Updated the PR to fix the noted short name violation of __.php (squashed into 1 commit)

@fooman fooman force-pushed the fooman:move-functions-2.2 branch from 4fcb412 to e8f5186 Jul 18, 2018

*
* @return \Magento\Framework\Phrase
*/
function __()

This comment has been minimized.

Copy link
@ihor-sviziev

ihor-sviziev Jul 18, 2018

Contributor

Wasn't idea to put this function in Magento/Framework/Phrase/ - \Magento\Framework\Phrase namespace?

This comment has been minimized.

Copy link
@fooman

fooman Jul 18, 2018

Author Contributor

I don't think that was proposed so far. Wouldn't namespacing the function mean all existing templates would need to be adjusted?

This comment has been minimized.

Copy link
@ihor-sviziev

ihor-sviziev Jul 20, 2018

Contributor

Let's wait @orlangur for review

@fooman

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

@orlangur any thing else to do here before it can be merged?

@fooman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@orlangur or @ihor-sviziev anything else needed here?

@orlangur

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

@fooman sorry for delay, started review almost immediately but didn't finish it. Will do now.

@@ -0,0 +1,22 @@
<?php

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 13, 2018

Contributor

Which rule was violated by __.php name?

This comment has been minimized.

Copy link
@fooman

fooman Aug 13, 2018

Author Contributor

It was actually the function name itself __() which triggered the violation. As this was previously not occurring I assumed it was related to the file name. It turned out that wasn't the case and that __() was always in violation but making the changes brought this up.

If you have a strong preference I can change the filename __function.php back to __.php.

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 14, 2018

Contributor

Please do so.

@@ -0,0 +1,4 @@
<?php
if (!function_exists('__')) {
require '__function.php';

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 13, 2018

Contributor

What is the use for such registration file?

This comment has been minimized.

Copy link
@fooman

fooman Aug 13, 2018

Author Contributor

The main idea of splitting this out is the following:

The standard way of installing Magento 2 should be through Composer. In this case you will get the __function.php included directly through the Composer autoloader - no further requires needed. Also this registration file would not be loaded.

Only if you clone from Github app/etc/NonComposerComponentRegistration.php will kick in and find this registration file which in turn will require __function.php.

This comment has been minimized.

Copy link
@fooman

fooman Aug 13, 2018

Author Contributor

@orlangur we could mark this file deprecated as well as it should get removed at the same time as app/functions.php

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 14, 2018

Contributor

Such file is a bit confusing, please just include __.php in registration.php of Magento Framework.

@@ -45,7 +45,8 @@
"Magento\\Framework\\": ""
},
"files": [
"registration.php"
"registration.php",
"Phrase/__function.php"

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 13, 2018

Contributor

I meant to just require __.php in registration of framework.

This comment has been minimized.

Copy link
@fooman

fooman Aug 13, 2018

Author Contributor

To me the final goal should be to use Composer to supply autoloaded files. Using registration.php is an intermediate step which we could do without. It also keeps the purpose of the registration file.

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 14, 2018

Contributor

Using registration.php is an intermediate

There is no need to do something differently until we get rid of them.

So, instead of including __.php separately just do it within registration.php.

if (!empty($argc) && is_array($argc[0])) {
$argc = $argc[0];
$vendorDir = require VENDOR_PATH;
if (!function_exists('__')) {

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 13, 2018

Contributor

Why not simply move such logic into __.php? It's better to mark app/functions.php as deprecated without any changes.

@fooman fooman force-pushed the fooman:move-functions-2.2 branch from 03a3aa7 to c8860f5 Aug 14, 2018

@fooman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

@orlangur okay changed this to how you suggested so that we can get this merged

@ihor-sviziev ihor-sviziev requested a review from orlangur Aug 16, 2018

@orlangur
Copy link
Contributor

left a comment

@fooman great! Even simpler than I thought it will be 👍

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

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

@magento-engcom-team magento-engcom-team merged commit c8860f5 into magento:2.2-develop Sep 6, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
magento-engcom-team pushed a commit that referenced this pull request Sep 6, 2018
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

Hi @fooman. Thank you for your contribution.
We will aim to release these changes as part of 2.2.7.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

@magento-engcom-team magento-engcom-team added this to Ready to Port in Ports Wanted Sep 6, 2018

@fooman fooman referenced this pull request Sep 7, 2018
0 of 4 tasks complete

@magento-engcom-team magento-engcom-team moved this from Ready to Port to Port to 2.3 Created in Ports Wanted Sep 7, 2018

@magento-engcom-team magento-engcom-team moved this from Port to 2.3 Created to Port to 2.3 Merged in Ports Wanted Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.