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

[Feature] Change to use Composer for autoload, drop PEAR/EZC #1340

Merged
merged 4 commits into from
Jan 15, 2018

Conversation

andrerom
Copy link
Contributor

@andrerom andrerom commented Jan 10, 2018

Mainly in order to solve the autloading issue with eZ Platform v2, but also:

  • Allows standalone usage in scripts using standard composer autoload
  • Automatically generates autoload on composer install / update (when legacy is root package. In legacy bridge setup, legacy bridge handles this and more)
  • Removes the dead code around PEAR and bundled ez Components which is not relevant anymore, and actually risky since they don't received (security) updates anymore

Result is more clean autoloading code which just have three cases to care about in regards to making sure things like Zeta Components are loaded.

autoload.php Outdated
@@ -19,6 +19,12 @@
require_once __DIR__ . '/config.php';
}

// If composer autoloaded context such as eZ Platform & legacy bridge, skip trying to autoload Zeta Components and such
if ( class_exists( 'Composer\Autoload\ClassLoader', false ) )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had forgot about that one.

It's more or less the same, so not needed then. Only difference is that this also detects usage in plain legacy. As in:

require __DIR__ . '/vendor/autoload.php';

$ini = eZINI::instance();
echo $ini->variable( 'SiteAccessSettings', 'ForceVirtualHost' );

That said I'm unsure if allowing yet another way to do things is good. So I'll gladly remove it.

Copy link
Contributor Author

@andrerom andrerom Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But main reason I added it is because I need it in order to be able to do the section in composer:

    "autoload": {
      "files": ["autoload.php"]
    },

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'll test the patch tomorrow to see how it behaves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, is there a point in using Composer autoloading in context of pure legacy, seeing how you just can do require autoload.php, instead of require vendor/autoload.php and achieve the exact same thing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emodric I imagine this allows using composer-installed dependencies in a pure-legacy context... An interesting concept - albeit probably not many legacy extensions are currently available that take advantage of this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

albeit probably not many legacy extensions are currently available that take advantage of this

@gggeek If any, hence, my question :) But I suppose there's no harm done in allowing Composer autoloading too.

Copy link
Contributor Author

@andrerom andrerom Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, is there a point in using Co

As said, this is not the main point* of this, just a side benefit/downside however you look at it. I know many legacy parts don't use compoer and never will, exhibit A: package system as used by setup wizard.

* Main point: To auto register legacy autoload in composer in a Platform + Legacy Bridge setup to get rid of dependency on app/autoload.php which was removed in Symfony 3 and hence also eZ Platform v2 as there is no need for it. Somewhat pure composer autoloading for the win 🎉

@emodric
Copy link
Collaborator

emodric commented Jan 11, 2018

The error about CLIHandler (ezsystems/LegacyBridge#132) is gone, but there's still an error when running the script to generate autoloads from eZ Platform root.

I think that eZ Publish Legacy still expects app/autoload.php to exist (https://github.com/ezsystems/ezpublish-legacy/blob/master/autoload.php#L48, https://github.com/ezsystems/ezpublish-legacy/blob/master/bin/php/ezpgenerateautoloads.php#L42), because when I create an empty app/autoload.php, the error is gone.

> eZ\Bundle\EzPublishLegacyBundle\Composer\ScriptHandler::generateAutoloads
Running script 'bin/php/ezpgenerateautoloads.php' in eZ Publish legacy context
PHP Warning:  require(Base/src/base.php): failed to open stream: No such file or directory in /media/eddie/data/html/netgensite/ezpublish_legacy/bin/php/ezpgenerateautoloads.php on line 58
PHP Fatal error:  require(): Failed opening required 'Base/src/base.php' (include_path='.:/usr/share/php') in /media/eddie/data/html/netgensite/ezpublish_legacy/bin/php/ezpgenerateautoloads.php on line 58
09:56:40 CRITICAL  [php] Fatal Compile Error: require(): Failed opening required 'Base/src/base.php' (include_path='.:/usr/share/php') ["exception" => Symfony\Component\Debug\Exception\FatalErrorException { …}] []

In ezpgenerateautoloads.php line 58:
                                                                               
  Compile Error: require(): Failed opening required 'Base/src/base.php' (incl  
  ude_path='.:/usr/share/php')                                                 
                                                                               

ezpublish:legacy:script [--legacy-help] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--siteaccess [SITEACCESS]] [--] <command> <script>

Script eZ\Bundle\EzPublishLegacyBundle\Composer\ScriptHandler::generateAutoloads handling the symfony-scripts event terminated with an exception

                                                                                                                                                                                                                                    
  [RuntimeException]                                                                                                                                                                                                                
  An error occurred when executing the "'ezpublish:legacy:script bin/php/ezpgenerateautoloads.php'" command:                                                                                                                        
                                                                                                                                                                                                                                    
  Running script 'bin/php/ezpgenerateautoloads.php' in eZ Publish legacy context                                                                                                                                                    
                                                                                                                                                                                                                                    
                                                                                                                                                                                                                                    
  PHP Warning:  require(Base/src/base.php): failed to open stream: No such file or directory in /media/eddie/data/html/netgensite/ezpublish_legacy/bin/php/ezpgenerateautoloads.php on line 58                                  
  PHP Fatal error:  require(): Failed opening required 'Base/src/base.php' (include_path='.:/usr/share/php') in /media/eddie/data/html/netgensite/ezpublish_legacy/bin/php/ezpgenerateautoloads.php on line 58                  
  09:56:40 CRITICAL  [php] Fatal Compile Error: require(): Failed opening required 'Base/src/base.php' (include_path='.:/usr/share/php') ["exception" => Symfony\Component\Debug\Exception\FatalErrorException { …}] []             
                                                                                                                                                                                                                                    
  In ezpgenerateautoloads.php line 58:                                                                                                                                                                                              
                                                                                                                                                                                                                                    
    Compile Error: require(): Failed opening required 'Base/src/base.php' (incl                                                                                                                                                     
    ude_path='.:/usr/share/php')                                                                                                                                                                                                    
                                                                                                                                                                                                                                    
                                                                                                                                                                                                                                    
  ezpublish:legacy:script [--legacy-help] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--siteaccess [SITEACCESS]] [--] <command> <script>  

Copy link
Collaborator

@emodric emodric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything works now

@alongosz
Copy link
Member

Hmm, trying to test that with v2 + LegacyBridge but in meta-repository (ezplatform v2) I still need to modify autoload section of composer.json to have:

"autoload": {
        "psr-4": {
            "AppBundle\\": "src/AppBundle/"
        },
        "files": ["ezpublish_legacy/autoload.php"],
        "classmap": [ "app/AppKernel.php", "app/AppCache.php" ]
    },

Does v2 + LegacyBridge setup works for you without it?

Also now when clearing v2 Sf cache I'm getting:

In ezini.php line 1437:
                                                                       
  injected-merge-settings can only reference and contain array values                                                                         

but this seems to be unrelated to changes, so just a side note.

@emodric
Copy link
Collaborator

emodric commented Jan 11, 2018

@alongosz Works fine without any changes to root composer.json. Did you actually install this branch through Composer with dev-composer_autoload_support as 2017.12.0 as a version in require section?

I've tested with https://github.com/emodric/ezplatform-legacy with emodric/ezplatform-legacy@ab8d3e0 reverted.

Also, no mention of ezini.php error.

@andrerom andrerom changed the title [Feature] Add support for autoloading via Composer [Feature] Change to use Composer for autoload, drop PEAR/EZC Jan 11, 2018
@andrerom
Copy link
Contributor Author

andrerom commented Jan 11, 2018

Worked for me her now too.

> eZ\Bundle\EzPublishLegacyBundle\Composer\ScriptHandler::generateAutoloads
Running script 'bin/php/ezpgenerateautoloads.php' in eZ Publish legacy context
Scanning for PHP-files.

Scan complete. Found 266 PHP files.

Searching for classes (tokenizing).

Found 108 classes, added 108 of them to the autoload array.

> eZ\Bundle\EzPublishLegacyBundle\Composer\ScriptHandler::symlinkLegacyFiles
Aborting: The src directory "src/legacy_files" does not exist.

@emodric
Copy link
Collaborator

emodric commented Jan 11, 2018

@andrerom Will this be tagged with 2017.12.1 ?

@emodric
Copy link
Collaborator

emodric commented Jan 11, 2018

P.S. Just discovered this, when running legacy scripts in pure legacy context:

eddie@abyss [~/www/ezpublish_legacy] ( ± composer_autoload_support ● ) $ php bin/php/flatten.php all   
PHP Notice:  Constant EZCBASE_ENABLED already defined in /media/eddie/data/html/ezpublish_legacy/autoload.php on line 44

@alongosz
Copy link
Member

@alongosz Works fine without any changes to root composer.json. Did you actually install this branch through Composer with dev-composer_autoload_support as 2017.12.0 as a version in require section?

@emodric I didn't think of that because I had dependency on LegacyBridge only, so just symlinked ezpublish_legacy directory to repo with these changes. I guess some composer workflow is missing in this case 😅

Anyway, when done properly as you've suggested - it works :)

P.S. Just discovered this, when running legacy scripts in pure legacy context

eddie@abyss [~/www/ezpublish_legacy] ( ± composer_autoload_support ● ) $ php bin/php/flatten.php all   
PHP Notice:  Constant EZCBASE_ENABLED already defined in ?>/media/eddie/data/html/ezpublish_legacy/autoload.php on line 44

It also is visible in unit tests on Travis, seems like autoload.php is included at least twice somewehere.

cc @andrerom

@emodric
Copy link
Collaborator

emodric commented Jan 11, 2018

seems like autoload.php is included at least twice somewehere.

Yep, looks like it. For example, if I change require_once 'autoload.php'; to require_once 'vendor/autoload.php'; in bin/php/flatten.php, no more notice.

@andrerom
Copy link
Contributor Author

Maybe it's too close to the weekend already, but like before it's surrounded by if ( !defined( 'EZCBASE_ENABLED' ) ), so have a hard time seeing why it would be defined twice actually 😕

@emodric
Copy link
Collaborator

emodric commented Jan 12, 2018

@andrerom I'm not so sure myself. I can confirm that the file is ran twice, but I can't figure out why or how it goes into that if clause.

BTW, with these changes, this too can be removed now: https://github.com/ezsystems/ezpublish-legacy/blob/master/config.php-RECOMMENDED#L23-L53, as those three constants are not used anymore.

@emodric
Copy link
Collaborator

emodric commented Jan 12, 2018

Debugging further, it seems that class_exists call triggers the autoloading mechanism, despite the second false argument.

@andrerom
Copy link
Contributor Author

andrerom commented Jan 12, 2018

Hmm, I'd like to try to double check that once I have the time. But if that is the case, I'll probably have to resort to using spl_autoload_functions() and check for instance of composer. Or change the logic. Suggestions welcome.

@emodric
Copy link
Collaborator

emodric commented Jan 12, 2018

It seems that I was wrong after all. The problem is not class_exists, but the fact that we're requiring vendor/autoload.php from autoload.php BEFORE EZCBASE_ENABLED is defined (lines 34 & 40), which again requires autoload.php due to it being declared in composer.json autoload section.

Hacking on vendor/composer/autoload_real.php to use require_once instead of require gets rid of the notice.

@andrerom
Copy link
Contributor Author

andrerom commented Jan 12, 2018

That make complete sense, sorry for sending you down the recursion rabbit hole 🙈
Anyway, hope that is fixed now with latests changes, also removed the not relevant parts in config.php as well 😃

@emodric
Copy link
Collaborator

emodric commented Jan 12, 2018

Thanks! I'll test soon 👍

@emodric
Copy link
Collaborator

emodric commented Jan 15, 2018

@andrerom Everything works fine now, so 👍 for merging.

@andrerom
Copy link
Contributor Author

@alongosz / @gggeek looks good now?

@andrerom andrerom merged commit ffaab4a into master Jan 15, 2018
@andrerom andrerom deleted the composer_autoload_support branch January 24, 2018 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants