Remove the conditional class declaration of ArrayObject #3684

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

ircmaxell commented Feb 6, 2013

Remove the conditional class declaration from ArrayObject
Always defining the custom class. This is because there's no
benefit to using the core one (only possible negatives).
Additionally, the conditional class declaration can play
havoc with opcode caches and certain extensions.

Remove conditional class declaration
Remove the conditional class declaration from ArrayObject
Always defining the custom class. This is because there's no
benefit to using the core one (only possible negatives).
Additionally, the conditional class declaration can play
havoc with opcode caches and certain extensions.
Contributor

mwillbanks commented Feb 6, 2013

Hey; glad to see you here. Unfortunately the reason for the conditional class is because of PHP 5.3.3 support. We were actually testing opcode caching today with this change as well and @weierophinney can provide more details there.

Basically all of this is due to ArrayAccess not having the facility to allow offsetGet to return by reference in PHP 5.3.3 (http://www.php.net/arrayaccess). If you look at the build: https://travis-ci.org/zendframework/zf2/jobs/4608438 from reverting this you will see the failures that it causes - a complete cluster fuck if you know what I mean. We had a huge discussion on the mailing list around if we bump the minor PHP version to 5.3.5 or not and basically the decision was to not do it until distribution support widens.

Moving along to ArrayObject... it is even worse as you cannot provide a reference to be returned via offsetGet; additionally any bug reports or the like opened up on it in the past have all been marked as "bogus" which leads to this crazy behavior of having to rewrite ArrayObject to retain some form of BC in userland extending ArrayAccess but then it only works in PHP 5.3.4+.

The other thought that we had was providing different classes by changing the autoloader to look for deprecated versions by a different extension or the like. You will see a few of these conditional blocks now in order to fix unsetting multi-dimensional arrays. If you have any additional ideas by all means bring them up :)

Contributor

ircmaxell commented Feb 6, 2013

Yeah, I realized that once I saw the test results from travis.

The way that I've solved this before is to use a bootstrap file to load polyfills. So you'd have a bootstrap file (that composer would include on every load) that would check the PHP version, and load the 5.3.3 version of the class (which lives in another directory, violating PSR-0 intentionally).

The benefit to doing that, is that you can transparently add any polyfill. For example, if you wanted to provide a core CallbackFilterIterator (which is only available in 5.4+), you could use the bootstrap mechanism to load it in. That way it auto-disables itself when you upgrade, and you don't need any funny logic in the autoloader.

Thoughts?

Owner

weierophinney commented Feb 6, 2013

@ircmaxell I like this idea. Can you provide links/references/etc on how to accomplish the composer side of things? We can document alternate workflows for those not using Composer fairly easily.

Owner

weierophinney commented Feb 6, 2013

I think I found it: http://getcomposer.org/doc/04-schema.md#files -- going to try and work something up now.

Contributor

ircmaxell commented Feb 6, 2013

@weierophinney I'm working on a PR now (proof of concept at least). Give me a few minutes and I'll update this here...

Contributor

ircmaxell commented Feb 6, 2013

@weierophinney I've updated this PR with the concept I posted above. I added a new root level folder called "compatibility" (the name can be tweaked if desired). Then I added a bootstrap.php file inside of it which does the conditional loading of the dependencies.

Future polyfills and version-dependent code could be added to that bootstrap.php file...

Thoughts?

Contributor

mwillbanks commented Feb 6, 2013

I like this; question now becomes how do we handle cases where someone is downloading components of zf2; aka zend-session zend-stdlib etc where these polyfills will be needed. Seems like any circumstance that we hit in this case we need to always load those classes.

We could likely check to see if the component is installed or check the directories for which items are actually there. There are actually 2 additional cases where these need to be handled:
Zend\Session\Container and Zend\Session\Storage\SessionArrayStorage since they override ArrayAccess::offsetGet.

Owner

weierophinney commented Feb 6, 2013

I was just playing with this as well, @ircmaxell -- and came up with the same issues as @mwillbanks. We'd need to put the compatibility in each component that has such issues, and have the file loaded in the component composer.json as well as the framework composer.json.

The bigger problem, though, is that Composer is not properly creating the path to the script -- it's making it relative to the vendor directory, not the package directory. I'm going to see if I can figure that out.

Contributor

ircmaxell commented Feb 6, 2013

Interesting.

Perhaps have the polyfill be a separate composer sub-package. That way, Stdlib would declare the polyfill package as a dependency. That SHOULD take care of this particular issue...

Or am I missing some part of it...

Owner

weierophinney commented Feb 6, 2013

Actually, the path is created correctly -- I was misinterpreting vendor/composer/autoload_real.php.

Owner

weierophinney commented Feb 6, 2013

@ircmaxell -- please wait on any further changes -- I'm working up a solution that will allow Stdlib to be shipped as a standalone package or as part of the framework.

Contributor

ircmaxell commented Feb 6, 2013

I've updated it so that it can be a separate package (with composer support), having Stdlib's composer require it...

weierophinney added a commit to weierophinney/zendframework that referenced this pull request Feb 6, 2013

[#3684] Polyfill support for version-dependent classes
- Per ideas from @ircmaxell, removed the conditional declaration of classes in
  favor of an autoloading solution. If using 5.3.3, composer will now load an
  alternate Zend\Stdlib\ArrayObject implementation.

  Both the project- and component-level composer.json files were modified so
  that the change will take place regardless of whether you install the full
  framework or just the component.

- Made changes to the php-code-sniffer configuration to ensure that the
  PHP-5.3.3-specific ArrayObject implementation is not checked, as it violates
  PSR-0 intentionally.

  - Realized that travis-build could be simplified, as .php_cs has the expected
    configuration for the full framework.

@ghost ghost assigned weierophinney Feb 6, 2013

weierophinney added a commit to weierophinney/zendframework that referenced this pull request Feb 6, 2013

[#3684] Do polyfill for Session component as well
- SessionArrayStorage and Container were both doing version-specific conditional
  declarations; moved these into a compat layer as was done in Stdlib, and
  updated the composer.json for each of the component and framework.
- Currently, the SessionConfigTest suite is not running for me, but appears to
  be a random autoloading error from running in isolated processes; will see how
  the Travis-CI run looks.
Contributor

ircmaxell commented Feb 6, 2013

Closing PR as [#3684] solves this better...

@ircmaxell ircmaxell closed this Feb 6, 2013

weierophinney added a commit to weierophinney/zendframework that referenced this pull request Feb 6, 2013

[#3684] Simplified .php_cs fixers declaration
- A constant exists for what I wanted... Thanks, @Maks3w

weierophinney added a commit to weierophinney/zendframework that referenced this pull request Feb 6, 2013

[#3684] Fix indentation
- Fixes docblock indentation in compat file

@GordonSchmidt GordonSchmidt referenced this pull request in zendframework/ZendSkeletonApplication Feb 7, 2013

Closed

SkeletonApplication with Zend Framework 2.1.1 #161

weierophinney added a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

[zendframework/zendframework#3684] Polyfill support for version-depen…
…dent classes

- Per ideas from @ircmaxell, removed the conditional declaration of classes in
  favor of an autoloading solution. If using 5.3.3, composer will now load an
  alternate Zend\Stdlib\ArrayObject implementation.

  Both the project- and component-level composer.json files were modified so
  that the change will take place regardless of whether you install the full
  framework or just the component.

- Made changes to the php-code-sniffer configuration to ensure that the
  PHP-5.3.3-specific ArrayObject implementation is not checked, as it violates
  PSR-0 intentionally.

  - Realized that travis-build could be simplified, as .php_cs has the expected
    configuration for the full framework.

weierophinney added a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

[zendframework/zendframework#3684] Fix indentation
- Fixes docblock indentation in compat file

weierophinney added a commit to zendframework/zend-session that referenced this pull request May 15, 2015

[zendframework/zendframework#3684] Do polyfill for Session component …
…as well

- SessionArrayStorage and Container were both doing version-specific conditional
  declarations; moved these into a compat layer as was done in Stdlib, and
  updated the composer.json for each of the component and framework.
- Currently, the SessionConfigTest suite is not running for me, but appears to
  be a random autoloading error from running in isolated processes; will see how
  the Travis-CI run looks.

weierophinney added a commit to zendframework/zend-i18n-resources that referenced this pull request May 28, 2015

[zendframework/zendframework#3684] Polyfill support for version-depen…
…dent classes

- Per ideas from @ircmaxell, removed the conditional declaration of classes in
  favor of an autoloading solution. If using 5.3.3, composer will now load an
  alternate Zend\Stdlib\ArrayObject implementation.

  Both the project- and component-level composer.json files were modified so
  that the change will take place regardless of whether you install the full
  framework or just the component.

- Made changes to the php-code-sniffer configuration to ensure that the
  PHP-5.3.3-specific ArrayObject implementation is not checked, as it violates
  PSR-0 intentionally.

  - Realized that travis-build could be simplified, as .php_cs has the expected
    configuration for the full framework.

weierophinney added a commit to zendframework/zend-i18n-resources that referenced this pull request May 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment