Better polyfill support for Stdlib and Session #3966

weierophinney · 2013-03-05T17:46:40Z

This PR aims to provide more robust, simpler-to-use polyfill support for Zend\Stdlib and Zend\Session.

The current support requires that uses call on a compatibility/autoload.php file in order for the polyfill replacements to be made. This creates problems:

Education. We have to educate those who use the component individually how to load that file, both with and without Composer.
Breaks classmap generation. Since the same class is declared in multiple files, classmap generation in either Composer or ZF's own classmap_generator.php fails, as the incorrect variant may be used.

The proposed solution uses PHP's class_alias. This function allows you to globally alias one class name to another. I have put class_alias statements inside conditionals, achieving conditional imports:

// In library/Zend/Stdlib/ArrayObject.php:

if (version_compare(PHP_VERSION, '5.4.0', 'lt')) {
    class_alias('Zend\Stdlib\ArrayObject\PhpLegacyCompatibility', 'Zend\Stdlib\AbstractArrayObject');
} else {
    class_alias('Zend\Stdlib\ArrayObject\PhpReferenceCompatibility', 'Zend\Stdlib\AbstractArrayObject');
}

class ArrayObject extends AbstractArrayObject
{
}

This approach means we no longer need to introduce autoloading hacks, and makes it possible for classmap generation to work properly again.

DASPRiD · 2013-03-05T17:57:13Z

library/Zend/Stdlib/ArrayObject.php

- * are unable to unset multi-dimensional arrays because you
- * need to fetch the properties / lists as references.
+ * If the version is less than 5.3.4, we'll use Zend\Stdlib\ArrayObject\PhpLegacyCompatibility
+ * which extend sthe native PHP ArrayObject implementation. For versions greater than or equal


Move the space between "extend sthe" :)

Maks3w · 2013-03-05T19:20:11Z

If this is not fixed any issue in the wild then may be better merge this against devel

weierophinney · 2013-03-05T19:22:44Z

If this is not fixed any issue in the wild then may be better merge this against devel

@Maks3w - It actually does fix issues reported in the wild:

Inability to generate autoloading classmaps
Developers unsure what they need to do to get the polyfill support when not using Composer

The solution presented in here fixes both of those, and makes usage as easy as use Zend\Stdlib\ArrayObject; $object = new ArrayObject(); and it simply works.

- Instead of using an autoload file, instead created two separate classes, named after the type of support they provide. The file Zend\Stdlib\ArrayObject now simply has a conditional in it; based on the condition met, a class_alias() is created -- and from then on, that alias will be used for Zend\Stdlib\ArrayObject. - Removed autoload.files entries from composer.json files; no longer necessary. - One test failure currently in doing a comparison of merged array objects; will investigate in a later commit.

- Created Zend\Session\Container and Zend\Session\Storage\SessionArrayStorage namespaces. Each contains PhpLegacyCompatibility and PhpReferenceCompatibility classes. The classes with the same name as the new namespaces each use conditionals to execute class_alias() commands in order to perform version-specific polyfills. - Removed compatibility/autoload.php from composer.json autoload.files; no longer necessary. - All tests are running!

- changed a typehint from "ArrayObject" to "self"

- bad import

- BC measure, so that code referencing these files doesn't break

@DASPRiD

- per @DASPRiD

- typehint on AbstractContainer - when instantiating a Container instance, use fully qualified class name in order to trigger alias

- Imports will work against class_alias - Typehinting works, *except* when using mocks, as mocks do not take into account class aliasing.

Freeaqingme · 2013-03-05T20:59:23Z

library/Zend/Session/Container/PhpReferenceCompatibility.php

+        }
+        $storage = $this->getStorage();
+        $name    = $this->getName();
+        $ret =& $storage[$name][$key];


Indentation

What's the change needed? (This is the original formatting from @mwillbanks -- just moved to a new file.)

@deprecated

- moved autoload.php files into compatibility subdirs where they belong - marked each with @deprecated annotation

Freeaqingme · 2013-03-05T21:04:48Z

library/Zend/Stdlib/ArrayObject.php

-        }
-    }
+if (version_compare(PHP_VERSION, '5.3.4', 'lt')
+    && !class_exists('Zend\Stdlib\ArrayObject', false)


Why do you check if that class was already defined?

If you want to ensure you only load it once, shouldn't you also use 'elseif' (checking for the same condition) instead of 'else'?

If we want to keep php's behavior as regular as possible, we shouldn't perform this check at all I think. Including the same file twice will usually generate a 'cannot redeclare class at...'

bad cut-and-paste -- I've removed it in a separate commit now.

- bad cut-and-paste

Freeaqingme · 2013-03-05T21:09:52Z

Review complete.

- Use __halt_compiler(), and then define stub class, in order to trick classmap compilers into thinking a class exists.

- Allows finding class definitions following halt_compiler directives, effectively allowing polyfill support in classmap generation.

weierophinney · 2013-03-05T23:27:35Z

@Freeaqingme and @DASPRiD I've done the following in the last few commits:

Added class definition stubs following __halt_compiler() directives in each polyfill class stub (the ones with the class_alias directives)
Updated Zend\File\ClassFileLocator to look for class definitions following halt_compiler tokens (this allows classmap_generator.php to work
In the compatibility/autoload.php files, I trigger an E_USER_DEPRECATED error
Added notes to the README.md about the changes

Ready to merge, if tests pass.

DASPRiD · 2013-03-06T01:39:34Z

I'm still wondering about one little thing before merging. I did a small test, and this also seems to work, not requiring any classmap generator changes or the like. @weierophinney, can you verify if this is a valid solution? I'm not sure if you considered this already, and/or if it also messes with opcode caching, but else it's probably worth taking a look at:

<?php
namespace Zend\Stdlib\ArrayObject;

if (version_compare(PHP_VERSION, '5.4.0', 'lt')) {
    class_alias('Zend\Stdlib\ArrayObject\PhpLegacyCompatibility', 'Zend\Stdlib\ArrayObjectBase');
} else {
    class_alias('Zend\Stdlib\ArrayObject\PhpReferenceCompatibility', 'Zend\Stdlib\ArrayObjectBase');
}

class ArrayObject extends ArrayObjectBase
{
}

- Have a base class that extends a polyfill class - makes classmap generation simpler - makes typehinting and mock objects simpler

- No longer necessary to do the halt_compiler hack

- No longer necessary

- To allow running ant build without prompting for missing files

- Changes are simpler, so simpler explanation is in order.

- they are not intended to be instantiated directly; they are intended as bases for the typehinted class that uses the polyfills.

weierophinney · 2013-03-06T12:37:00Z

@DASPRiD Your comment is exactly the reason I was looking for review! The solution you outline both simplifies the support, and solves the typehinting problems. I've implemented that now, and rolled back the changes to the CSRF validator and to the ClassFileLocator.

- since the "legacy" class simply extended AbstractContainer, can use that as the polyfill

- It was an empty extension of AbstractSessionArrayStorage; simply use that class as the polyfill

weierophinney · 2013-03-06T13:12:08Z

I was able to simplify even further, as two of the Session polyfills were simply empty extensions of existing abstract classes.

@DASPRiD @Freeaqingme @Maks3w Ready for final review.

@mwillbanks -- check out the solution -- it's basically the conditional use statements we wanted originally. :)

DASPRiD · 2013-03-06T13:18:12Z

Looks very good to me now. Preferring a second review though before merging :)

mwillbanks · 2013-03-06T14:54:22Z

@weierophinney this looks good 👍 I like the conditionals TBH, much more explicit and easier to determine. Also far happier with this in the end as it basically does what we intend it to do.

Freeaqingme · 2013-03-06T17:23:08Z

README.md

@@ -13,6 +13,26 @@ DD MMM YYYY

 ### UPDATES IN 2.1.4

+Better polyfill support in `Zend\Session` and `Zend\Stdlib`. Polyfills


Shouldn't this paragraph have some kind of caption or title? If we add more to this readme, it would ease on legibility. Although I'm not entirely sure this is merely hypothetical.

Also, this is part of the changelog for 2.1.4. Shouldn't we also add it to the change log for 2.2, so that people who upgrade from minor to minor release get to see the list of changes? (I'm not sure discussing this on this PR is the right place to do so though).

This will go into CHANGELOG.md when we release 2.1.4, and that always gets merged back to develop as well.

IF we have more updates we want to call out, we can add additional subheaders; with only one, there's no need yet.

Freeaqingme · 2013-03-06T17:24:57Z

@weierophinney Looks good! There is one mini remark about indentation that it would seem you missed. Remarks on the changelog may have to be discussed elsewhere, independent from putting this PR forward.

…elop PR #3966

Freeaqingme · 2013-03-06T21:43:59Z

Merged in 1d812e5 (develop) and a9da694 (master)

…elop PR zendframework/zendframework#3966

DASPRiD reviewed Mar 5, 2013
View reviewed changes

weierophinney added 8 commits March 5, 2013 13:31

Solved failing test in ArrayObject tests

aaedc2a

- changed a typehint from "ArrayObject" to "self"

Fix failing Zend\Session test under 5.3.3

1232872

- bad import

Re-added autoload.php files

3bf2855

- BC measure, so that code referencing these files doesn't break

Fix typo

843ac05

- per @DASPRiD

Fix polyfill support issue in CSRF validator

05bc3b1

- typehint on AbstractContainer - when instantiating a Container instance, use fully qualified class name in order to trigger alias

Typehint on AbstractContainer, but instantiate Container

41dbee9

- Imports will work against class_alias - Typehinting works, *except* when using mocks, as mocks do not take into account class aliasing.

Freeaqingme reviewed Mar 5, 2013
View reviewed changes

Renamed compat autoload files and marked as deprecated

d1e7d9d

- moved autoload.php files into compatibility subdirs where they belong - marked each with @deprecated annotation

Freeaqingme reviewed Mar 5, 2013
View reviewed changes

Remove class_exists check

010f534

- bad cut-and-paste

weierophinney added 6 commits March 5, 2013 15:37

Fake scanners out

a94f497

- Use __halt_compiler(), and then define stub class, in order to trick classmap compilers into thinking a class exists.

Class stubs to force classmap generation

5ead25d

Allow finding class definitions following halt_compiler

e206193

- Allows finding class definitions following halt_compiler directives, effectively allowing polyfill support in classmap generation.

Added note in README about new polyfill support

00ee382

Updated README to detail full change for polyfills

2af4f12

Trigger E_USER_DEPRECATED in deprecated polyfill autoload files

ee14182

weierophinney added 4 commits March 6, 2013 06:23

Simpler polyfill support

e6724fc

- Have a base class that extends a polyfill class - makes classmap generation simpler - makes typehinting and mock objects simpler

Revert changes to ClassFileLocator

7aededd

- No longer necessary to do the halt_compiler hack

Revert changes to CSRF validator

5761918

- No longer necessary

Added "build/" to gitignore

ecd744f

- To allow running ant build without prompting for missing files

weierophinney added 2 commits March 6, 2013 06:32

Updated README

9a6cde7

- Changes are simpler, so simpler explanation is in order.

Mark all polyfill bases as abstract

eb3f0cc

- they are not intended to be instantiated directly; they are intended as bases for the typehinted class that uses the polyfills.

weierophinney added 2 commits March 6, 2013 07:08

Don't need an additional polyfill class for Container

049d94c

- since the "legacy" class simply extended AbstractContainer, can use that as the polyfill

Do not need 5.3.3 polyfill for SessionArrayStorage

a1adc02

- It was an empty extension of AbstractSessionArrayStorage; simply use that class as the polyfill

Freeaqingme reviewed Mar 6, 2013
View reviewed changes

Freeaqingme merged commit a1adc02 into zendframework:master Mar 6, 2013

Freeaqingme added a commit that referenced this pull request Mar 6, 2013

Merge branch 'weierophinney-hotfix/polyfill-via-class-alias' into dev…

1d812e5

…elop PR #3966

weierophinney deleted the hotfix/polyfill-via-class-alias branch March 6, 2013 21:49

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

Merge branch 'weierophinney-hotfix/polyfill-via-class-alias' into dev…

e093860

…elop PR zendframework/zendframework#3966

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

Merge branch 'weierophinney-hotfix/polyfill-via-class-alias' into dev…

55d0483

…elop PR zendframework/zendframework#3966

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

Merge branch 'weierophinney-hotfix/polyfill-via-class-alias' into dev…

7c09779

…elop PR zendframework/zendframework#3966

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better polyfill support for Stdlib and Session #3966

Better polyfill support for Stdlib and Session #3966

weierophinney commented Mar 5, 2013

DASPRiD Mar 5, 2013

weierophinney Mar 5, 2013

Maks3w commented Mar 5, 2013

weierophinney commented Mar 5, 2013

Freeaqingme Mar 5, 2013

weierophinney Mar 6, 2013

Freeaqingme Mar 5, 2013

weierophinney Mar 5, 2013

Freeaqingme commented Mar 5, 2013

weierophinney commented Mar 5, 2013

DASPRiD commented Mar 6, 2013

weierophinney commented Mar 6, 2013

weierophinney commented Mar 6, 2013

DASPRiD commented Mar 6, 2013

mwillbanks commented Mar 6, 2013

Freeaqingme Mar 6, 2013

weierophinney Mar 6, 2013

Freeaqingme commented Mar 6, 2013

Freeaqingme commented Mar 6, 2013

		@@ -13,6 +13,26 @@ DD MMM YYYY

		### UPDATES IN 2.1.4

		Better polyfill support in `Zend\Session` and `Zend\Stdlib`. Polyfills

Better polyfill support for Stdlib and Session #3966

Better polyfill support for Stdlib and Session #3966

Conversation

weierophinney commented Mar 5, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maks3w commented Mar 5, 2013

weierophinney commented Mar 5, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freeaqingme commented Mar 5, 2013

weierophinney commented Mar 5, 2013

DASPRiD commented Mar 6, 2013

weierophinney commented Mar 6, 2013

weierophinney commented Mar 6, 2013

DASPRiD commented Mar 6, 2013

mwillbanks commented Mar 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freeaqingme commented Mar 6, 2013

Freeaqingme commented Mar 6, 2013