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

Already on GitHub? Sign in to your account

[#3684] Polyfill support for version-dependent classes #3689

Merged
merged 4 commits into from Feb 6, 2013

Conversation

Projects
None yet
4 participants
Owner

weierophinney commented Feb 6, 2013

  • 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
    alternate Zend\Stdlib\ArrayObject, Zend\Session\Container, and Zend\Session\Storage\SessionArrayStorage implementations.

    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 compatibility implementations are not checked, as they violate
    PSR-0 intentionally.

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

This builds on the ideas in #3684 and addresses the concerns about having the solution work for the individual component install versus a full framework install, and addresses the Session component as well.

@weierophinney weierophinney [#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.
834d631
Contributor

ircmaxell commented Feb 6, 2013

The only comment I have is that autoload.php is perhaps not the best name for said file... Since it's not really "autoloading" in the traditional sense... It's not a huge issue, but perhaps could be bootstrap or something of the like.

A minor quibble at best...

@weierophinney weierophinney [#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.
352a040
Owner

weierophinney commented Feb 6, 2013

The only comment I have is that autoload.php is perhaps not the best name for said file

Well, in a sense, it is autoloading -- it's loading one or more class files to ensure that later autoloaders will not be invoked. "bootstrap" is an overloaded term in the framework, unfortunately, as it is generally associated with the MVC bootstrap.

As you said -- minor issue. I'm okay with changing it if somebody has a really compelling recommendation.

Owner

weierophinney commented Feb 6, 2013

Waiting on the status of this before recommending for merge: https://travis-ci.org/zendframework/zf2/builds/4621355

Contributor

ircmaxell commented Feb 6, 2013

Sounds great to me! And agree that it's a minor issue (barely worth bringing up). I'll close my original PR... Thanks!

Member

Maks3w commented Feb 6, 2013

Composer files key doesn't look well for performance, that files are loaded always

If you want to require certain files explicitly on every request then you can use the 'files' autoloading mechanism. This is useful if your package includes PHP functions that cannot be autoloaded by PHP.
http://getcomposer.org/doc/04-schema.md#files

@Maks3w Maks3w and 1 other commented on an outdated diff Feb 6, 2013

$finder = Symfony\CS\Finder\DefaultFinder::create()
->notName('TestSampleClass10.php')
->exclude('demos')
- ->in(__DIR__);
-return Symfony\CS\Config\Config::create()
- ->finder($finder);
+ ->exclude('resources')
+ ->filter(function (SplFileInfo $file) {
+ if (strstr($file->getPath(), 'compatibility')) {
+ return false;
+ }
+ })
+ ->in(__DIR__ . '/library')
+ ->in(__DIR__ . '/tests')
+ ->in(__DIR__ . '/bin');
+$config = Symfony\CS\Config\Config::create();
+$config->fixers(array(
@Maks3w

Maks3w Feb 6, 2013

Member

$config->fixers(\Symfony\CS\FinderInterface::PSR2_LEVEL) is enough

@weierophinney

weierophinney Feb 6, 2013

Owner

I'll make that change -- thanks!

@weierophinney

weierophinney Feb 6, 2013

Owner

FinderInterface? or FixerInterface?

Owner

weierophinney commented Feb 6, 2013

@Maks3w Right -- but that's the behavior we want. We want to slip in our alternate implementations so that we don't have conditional class declaration (which plays poorly with opcode caches).

Additionally, with an opcode cache in place, that file will be loaded once, and then be in memory for all subsequent requests. It's a reasonable solution.

@Maks3w Maks3w commented on an outdated diff Feb 6, 2013

library/Zend/Stdlib/compatibility/ArrayObject.php
+namespace Zend\Stdlib;
+
+use ArrayObject as PhpArrayObject;
+
+/**
+ * ArrayObject
+ *
+ * Since we need to substitute an alternate ArrayObject implementation for
+ * versions > 5.3.3, we need to provide a stub for 5.3.3. This stub
+ * simply extends the PHP ArrayObject implementation, and provides default
+ * behavior in the constructor.
+ */
+class ArrayObject extends PhpArrayObject
+{
+ /**
+ * Constructor
@Maks3w

Maks3w Feb 6, 2013

Member

check the indentation

weierophinney added some commits Feb 6, 2013

@weierophinney weierophinney [#3684] Simplified .php_cs fixers declaration
- A constant exists for what I wanted... Thanks, @Maks3w
732360c
@weierophinney weierophinney [#3684] Fix indentation
- Fixes docblock indentation in compat file
0b88086
Owner

weierophinney commented Feb 6, 2013

Travis stalled on the PR build, but the build on my branch passed: http://travis-ci.org/weierophinney/zf2/builds/4621350

Changes made after that were to the .php_cs file (those changes work) and to indentation on a compat file; in other words, they will not change the build status.

Ready for review.

Contributor

mwillbanks commented Feb 6, 2013

Looks awesome 👍 Thanks @ircmaxell for the suggestion and @weierophinney for the implementation! Happy to have this no longer inside of the actual classes!

@Maks3w Maks3w added a commit that referenced this pull request Feb 6, 2013

@Maks3w Maks3w Merge pull request #3689 branch 'hotfix/3684' e9a25a5

@Maks3w Maks3w merged commit 0b88086 into zendframework:master Feb 6, 2013

@gianarb gianarb pushed a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

@Maks3w Maks3w Merge pull request zendframework/zendframework#3689 branch 'hotfix/3684' 96cb3fe

@gianarb gianarb pushed a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

@Maks3w Maks3w Forward zendframework/zendframework#3689 c0baffe

@gianarb gianarb pushed a commit to zendframework/zend-session that referenced this pull request May 15, 2015

@Maks3w Maks3w Merge pull request zendframework/zendframework#3689 branch 'hotfix/3684' 9533639

@gianarb gianarb pushed a commit to zendframework/zend-session that referenced this pull request May 15, 2015

@Maks3w Maks3w Forward zendframework/zendframework#3689 6f1de5e

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

@Maks3w Maks3w Merge pull request zendframework/zendframework#3689 branch 'hotfix/3684' a41d135

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

@Maks3w Maks3w Forward zendframework/zendframework#3689 7c61995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment