Remove deprecated and legacy code, migrate some syntax, cleanup #908

robfrawley · 2017-04-13T02:06:16Z

Q	A
Branch?	2.0
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

This PR removes all obviously marked deprecations and legacy code/tests from the 1.x branch for the 2.x branch, removing backward compatibility layers for older Symfony versions, and moves from legacy syntaxes to more modern solutions, such as from the long to short array syntax.

Requirements

Symfony >= 2.8
PHP >= 7.0

Todo Still

Change to short array syntax
Remove obviously marked deprecations
Remove obviously marked legacy code/text
Remove unobvious deprecations
Remove unobvious legacy code/texts
Update UPGRADE.md with all changes
Update any relevant documentation

Should we Go Further

Do we want to bump one more version for PHP and require PHP 7.0, which would allow for us to really clean up the code with function types explicitly mapped everywhere. IDK if it's worth it, but it's a thought.

Thanks to some comments, previous discussions, and my own thoughts, ATM this PR does go a step further to requiring PHP 7.x.

The only consideration we need to make, with a move to 7.x, is moving HHVM to an "allowed failure" for the time being. This is required due to HHVM's incorrect handling of language-level type checking, as well as globally-enabled function type checking, (in stark contrast to how PHP handled this), which breaks our code and some dependencies when running HHVM in PHP 7 mode. See the following issues for reference:

I've worked around this a bit by explicitly disabling the "type checking" feature of HHVM via the INI setting, but other errors still persist in PHP7-mode, so IDK.

robfrawley · 2017-04-13T02:11:44Z

Ooh yeah; Travis test time down from 13-15 minutes to 4 minutes!

alexwilson · 2017-04-16T13:39:38Z

There's a lot to this diff so it'll take me a while to go through in detail, but most immediately looking through how much more ... readable the testing configuration now is ... thank you so much!

rpkamp · 2017-04-23T05:12:00Z

Seeing how PHP 5.6 is officially end of life, I don't see a reason to support it anymore in 2.0.

Also Symfony 4 will also require PHP 7.0 (https://medium.com/@fabpot/fabien-potencier-4574622d6a7e) and Monolog 2.x will also require PHP 7.0 (https://github.com/Seldaek/monolog/blob/master/composer.json)

rpkamp · 2017-04-23T05:24:06Z

DependencyInjection/LiipImagineExtension.php

@@ -25,12 +25,12 @@ class LiipImagineExtension extends Extension
    /**
     * @var ResolverFactoryInterface[]
     */
-    protected $resolversFactories = array();
+    private $resolversFactories = array();


array() could be replaced with []

rpkamp · 2017-04-23T05:24:13Z

DependencyInjection/LiipImagineExtension.php


    /**
     * @var LoaderFactoryInterface[]
     */
-    protected $loadersFactories = array();
+    private $loadersFactories = array();


array() could be replaced with []

rpkamp · 2017-04-23T05:33:37Z

Templating/FilterExtension.php

+    public function getFilters()
+    {
+        return [
+            new \Twig_SimpleFilter('imagine_filter', array($this->helper, 'filter')),


can be replaced with [$this->helper, 'filter']

rpkamp · 2017-04-23T05:36:56Z

Templating/FilterExtension.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getName()


Twig_Extension#getName is deprecated and should be removed.

See https://github.com/twigphp/Twig/blob/2.x/CHANGELOG#L153

UPDATE: It seems Symfony/FrameworkBundle for 2.8 requires at least Twig 1.23 so this comment is invalid. Please ignore it.

robfrawley · 2017-05-02T17:10:37Z

Rebased to fix merge conflicts introduced by #914 (that PR removed all class name parameters from the service definition file).

@antoligy @cedricziel Aside from updating the UPGRADE.md and CHANGELOG.md files, this is ready to go. Things to consider before finalizing this PR:

Are we comfortable making the 2.x branch PHP 7.x only? Benefits include a vastly simplified Travis test definition file, as well as allowing us to introduce function type hints (which, IMHO, help with usage, debugging, and provide a great firewall between an improper use of a method), as well as some other language constructs. Moreover, as was already pointed out, the next major Symfony version will require PHP 7.x and I foresee releasing 2.x around the same timeframe (Novemberish). Of course, the 1.x branch will remain as is for those who need to use PHP 5.6, and I intend to continue to accept PRs and issues against it for some time, so it's not like people won't be able to use this bundle with older PHP versions...thoughts?
The Mongo functionality was removed due to it no longer working (the same extension isn't going to be made available for PHP 7.x); do we want to rewrite it instead of removing it (for the new extension)?

Aside from getting your opinion on the above items, I'd really appreciate it if you guys could take some time to review some of the changes introduced by the PR. I have other PRs that will start architecturally changing this bundle on the 2.x branch, but I don't want to add those until we've got this completed to avoid merge conflicts.

(If anyone else involved in this discussion has time to review this PR, that would greatly appreciated as well; this is a large PR so the more eyes the better @rpkamp @Koc)

robfrawley · 2017-05-02T17:14:15Z

Binary/Loader/FileSystemLoader.php

-        if (func_num_args() >= 4 && false === ($this->locator = func_get_arg(3)) instanceof LocatorInterface) {
-            throw new \InvalidArgumentException(sprintf('Method %s() expects a LocatorInterface for the forth argument.', __METHOD__));
-        } elseif (func_num_args() < 4) {
-            @trigger_error(sprintf('Method %s() will have a forth `LocatorInterface $locator` argument in version 2.0. Not defining it is deprecated since version 1.7.2', __METHOD__), E_USER_DEPRECATED);


Note to self: This deprecation notice needs to be amended to properly articulate the changes.

Koc · 2017-05-02T17:16:19Z

+1 for PHP 7.x or even 7.1 for nullable return types

rpkamp · 2017-05-03T19:19:48Z

Binary/Loader/FileSystemLoader.php

-
-        $this->locator->setOptions(array('roots' => (array) $dataRoots));
+        $this->locator = $locator;
+        $this->locator->setOptions(['roots' => $rootPaths]);


Why pass the $rootPaths to this class anyway since it's only handing them off to the passed in LocatorInterface? Why not set the option on the LocatorInterface before passing it to this class?

That is a valid question and something I've considered changing, but the current configuration handling of these items is tied to the *Loader implementations and not the *Locator classes; the FileLocator and InsecureFileSystem "locator" variants are really implementation details ATM.

I'm not really positive how we should handle this moving forward and am very open to its discussion, but I do think that it is more appropriate to handle that in a PR of its own, no?

IDK: now you got me thinking maybe it should be fully handled in this PR; what do you think?

Since you're making a signature change here I would solve the problem fully in this PR, instead of in another PR which breaks the signature a second time. People aren't supposed to be using 2.x yet I guess, but you never know 😃

I have not found a straightforward solution to this, and based on my testing the solution will likely be quite large (only making this pull request even larger), so I don't forsee this issue being resolved by this pull request. See #908 (comment) for additional details.

As for breaking changes (multiple times), I'm not too concerned about that; the whole point of having a development branch is to be able to actively develop on it. Those running the 1.x branch should expect no backward incompatible changes, but that doesn't apply here. I run the 2.0 branch on a few personal projects, and while it's annoying to update it when breaking changes occur, that is a contract I agreed to when I decided to run an unreleased version. We're going to need to introduce a plethora of additional breaking changes to complete many of the TODO items for 2.0, so let's focus on making the release amazing and not on making it easier for those running unreleased software. Right?

rpkamp · 2017-05-03T19:42:44Z

DependencyInjection/Factory/Resolver/WebPathResolverFactory.php


-        $container->setDefinition($resolverId, $resolverDefinition);
+        $container->setDefinition($resolverId = 'liip_imagine.cache.resolver.'.$resolverName, $resolverDefinition);


I would not make this change, it makes the code harder to read.

We do this often in the codebase now and can be found all over the place. While I disagree it makes it harder to read, I recognize many people feel this way. We should likely adopt a CONTRIBUTING.md file that defines some coding standards that the community agrees on as a whole.

rpkamp · 2017-05-03T19:45:17Z

DependencyInjection/LiipImagineExtension.php

@@ -57,7 +57,7 @@ public function getConfiguration(array $config, ContainerBuilder $container)
    }

    /**
-     * @see Symfony\Component\DependencyInjection\Extension.ExtensionInterface::load()
+     * @see \Symfony\Component\DependencyInjection\Extension.ExtensionInterface::load()


Why add the leading backslash here?

This is being migrated everywhere (slowly, they are everywhere) as many IDEs try to resolve these as within the current namespace when they are not declared as a root FQCN, instead of how they should be resolved, as true FQCN.

Excellent point, and there's more on how php docblocks FQCNs are supported here: https://phpdoc.org/docs/latest/guides/types.html#class-name

rpkamp · 2017-05-03T19:45:57Z

DependencyInjection/LiipImagineExtension.php

-            $factory = $this->resolversFactories[$factoryName];
-
-            $factory->create($container, $resolverName, $resolverConfig[$factoryName]);
+            $this->resolversFactories[$factoryName = key($resolverConfig)]->create($container, $resolverName, $resolverConfig[$factoryName]);


assigning and using a variable in the same line makes it harder to read.

Amended code to negate this issue

rpkamp · 2017-05-03T19:46:40Z

DependencyInjection/LiipImagineExtension.php

-                $definition->setFactoryClass($factory[0]);
-                $definition->setFactoryMethod($factory[1]);
-            }
+            $this->loadersFactories[$factoryName = key($loaderConfig)]->create($container, $loaderName, $loaderConfig[$factoryName]);


assigning and using a variable in the same line makes it harder to read.

Amended code to negate this issue

rpkamp · 2017-05-03T19:55:27Z

Resources/config/imagine.xml

            <tag name="twig.extension" />
-            <argument type="service" id="liip_imagine.cache.manager" />
+            <argument type="service" id="liip_imagine.templating.filter_helper" />


Why couple the twig helper to the templating helper? I realise the mostly have the same code, but making one dependent on the other feels off. If there is an abstraction here I would say that both the twig extension and the templating helper should depend on a shared service.

The intention was to remove code duplication; now that we've bumped the PHP requirements, though, traits serve the same purpose, so this has been rectified in that was, allowing both the helper and extension to again consume the CacheManager.

robfrawley · 2017-05-03T21:01:43Z

@rpkamp Thanks for the review!

Koc · 2017-05-04T07:45:42Z

DependencyInjection/Factory/ChildDefinitionTrait.php

+     */
+    private function getChildDefinition($parent)
+    {
+        return class_exists('\Symfony\Component\DependencyInjection\ChildDefinition') ?


you can use ChildDefinition::class here

Actually you can't, because if the class doesn't exist (in case of older Symfony versions) that can't be resolved.

it is safe for non-existent classes

http://blog.kevingomez.fr/til/2016/01/15/php-class-keyword/

Updated all FQCN strings to the special ::class compile-time constant: ee9c834. Thanks for the recommendation.

robfrawley · 2017-05-05T21:25:33Z

All the changes introduced by the pull request, as well as #914, have been added to the UPGRADE.md file in commit f04ffa3. @antoligy @cedricziel Any comments before this is merged?

alexwilson · 2017-05-06T15:10:16Z

On the topic of PHP 7, the primary argument to keeping our requirements lower were to match up with Symfony 2.8.

Since we've handled deprecations in 2.8 in the 1.0 branch anyway, now that we're raising to PHP 7 I'm honestly wondering if we ought to bump to Symfony 3.0?

alexwilson

Thanks for taking the trouble to go through and assemble this PR @robfrawley, this is a hefty chunk of the 2.0 roadmap and I cannot emphasise enough how useful all this is.

I've gone through with a more finely toothed comb, and all seems well, save for a few trivial things mostly housekeeping related...

This to me raises a few points to consider, potentially separately to this PR (because I would very much like to see it merged!):

Do we know, or have a way of telling, who uses the adapters that we provide? I am thinking primarily of GridFS and wondering if the replacement/alternative (should one even be required, my bias is that I do not yet use it!) could be potentially handled by a separate library.
Similarly, do we know if anybody uses this library running in a Hack environment? Have we just broken it with this suite of changes? (looks unlikely, given your reports on the testing setup and the linked issues)
Smaller, and more frequent PRs for the future? I understand that a lot of this was similar-ish and had to be within the same PR, but anything we can do to reduce the size of individual diffs is a win!

There's a trend of metrics here, unfortunately I haven't been able to find any useful information in Packagist... Possibly something to raise in a further issue, because 2.0 gives us the chance to trial feature apoptosis.

alexwilson · 2017-05-06T15:12:42Z

Utility/Framework/SymfonyFramework.php

@@ -18,24 +18,6 @@ class SymfonyFramework
    /**
     * @return bool
     */
-    public static function hasDefinitionSharing()


Not that anybody should have bound to this (or has had the time), but, should we deprecate this instead of flat-out removing?

Either way I think we ought to reference flag removal in the UPGRADE doc.

Looks like I missed this in the UPGRADE.md file; it should absolutely be mentioned. As for removal vs depreciation, I believe we should take advantage of not having a 2.x release yet and remove it.

Without a 2.x release, those running this development branch should know the API isn't stable. The moment we have an initial 2.0.0 release, this changes and we absolutely need to deprecate, but as of now, we should take the ability to remove outright and do so over depreciation, IMHO.

Definitely agreed about the 2.x API being unstable so it's ok to break atm until we are settled, we'd also be doing this by killing off the GridFS driver. 🙂
Happy to brush this one up then!

alexwilson · 2017-05-06T15:16:47Z

Tests/Imagine/Cache/SignerTest.php

@@ -21,9 +22,9 @@ class SignerTest extends AbstractTest
 {
    public function testImplementsSignerInterface()


This comment is not intended for action/review...

Now that I look at it, this is a very strange thing to test for! We typehint on SignerInterface, don't we?

(And all the others like it)

I always thought these tests were odd as well; 👍 remove them in another PR.

alexwilson · 2017-05-06T15:28:18Z

Binary/Loader/GridFSLoader.php

-use Doctrine\ODM\MongoDB\DocumentManager;
-use Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException;
-
-class GridFSLoader implements LoaderInterface


I think so, but I'm not sure that this should hold back this PR. Let's add to the roadmap for 2.0?

robfrawley · 2017-05-06T17:22:15Z

@antoligy

Do we know, or have a way of telling, who uses the adapters that we provide? I am thinking primarily of GridFS and wondering if the replacement/alternative (should one even be required, my bias is that I do not yet use it!) could be potentially handled by a separate library.

We don't and have no way of knowing. I don't use it either. I assume we'll have a PR shortly to add an adapter for the new mongodb extension, but as I don't use it I'm not particularly qualified to create the adapter myself.

Similarly, do we know if anybody uses this library running in a Hack environment? Have we just broken it with this suite of changes? (looks unlikely, given your reports on the testing setup and the linked issues)

Again, I don't know how we'd get access to any statistics. That said, the HHVM (in PHP 7 mode) issues extend outside this bundle to Composer and Symfony components, as well, so I do not believe anyone can relying on this bundle under those circumstances. Once HHVM fixes these core language issues, I'll be sure to enable support in Travis again, though it seems like many projects are leaving HHVM support now that PHP 7 was released and offers similar performance improvements. Sadly, the current state of HHVM makes it impossible to support PHP 7 and HHVM at the same time.

Smaller, and more frequent PRs for the future? I understand that a lot of this was similar-ish and had to be within the same PR, but anything we can do to reduce the size of individual diffs is a win!

Agreed; this blew up quick in size due to how many changes affected many files, but generally I agree with you here.

Since we've handled deprecations in 2.8 in the 1.0 branch anyway, now that we're raising to PHP 7 I'm honestly wondering if we ought to bump to Symfony 3.0?

I'm comfortable with Symfony 3.x as our 2.x base requirement.

- use short array syntax - update composer requirements to PHP 7+ and Symfony 3+ - migrate fqcn strings to ::class compile-time constant - move travis operations to bash scripts

robfrawley · 2017-05-06T20:35:31Z

I plan to merge this pull request at the end of today, now that we've had multiple reviews and all changes have been implemented, pending any objections from @antoligy, @cedricziel, @rpkamp or @Koc.

Last chance to make additional changes!

rpkamp · 2017-05-07T08:18:05Z

What about https://github.com/liip/LiipImagineBundle/pull/908/files#diff-af283426a4b2643990ccbcbae0d7555fR53? Will you fix it in this PR or should it be fixed later? Either way is fine by me 😃

robfrawley · 2017-05-07T16:39:38Z

@rpkamp Moving that should be handled in a subsequent pull request.

I'm not positive how to solve this issue, and it will only make this huge pull request even larger as solving the issue will likely span many files. Can you take a look and submit a pull request; a few quick attempts I made failed and it's not a priority for me ATM.

rpkamp · 2017-05-07T16:56:10Z

I will have a look once this PR is fixed, no problem

robfrawley added this to the 2.0.0 milestone Apr 13, 2017

robfrawley self-assigned this Apr 13, 2017

robfrawley changed the title ~~[WIP] Remove deprecations, legacy code, etc~~ [WIP] [RFC] Remove deprecations, legacy code, etc Apr 13, 2017

robfrawley requested review from cedricziel and alexwilson April 13, 2017 02:06

rpkamp reviewed Apr 23, 2017

View reviewed changes

robfrawley force-pushed the feature-deprecation-and-legacy-removal branch 8 times, most recently from 2872097 to 714be65 Compare April 24, 2017 04:49

robfrawley force-pushed the feature-deprecation-and-legacy-removal branch from 714be65 to f97a3cd Compare May 2, 2017 16:57

robfrawley commented May 2, 2017

View reviewed changes

robfrawley changed the title ~~[WIP] Remove deprecated and legacy code, migrate syntax to PHP 7+~~ [WIP] Remove deprecated and legacy code, migrate some syntax May 3, 2017

robfrawley removed the RFC: Draft This indicates the described RFC is still being drafted. label May 3, 2017

rpkamp reviewed May 3, 2017

View reviewed changes

robfrawley force-pushed the feature-deprecation-and-legacy-removal branch from 83b7646 to 279504d Compare May 3, 2017 21:12

robfrawley changed the title ~~[WIP] Remove deprecated and legacy code, migrate some syntax~~ [WIP] Remove deprecated and legacy code, migrate some syntax, cleanup May 3, 2017

Koc reviewed May 4, 2017

View reviewed changes

alexwilson approved these changes May 6, 2017

View reviewed changes

robfrawley force-pushed the feature-deprecation-and-legacy-removal branch from f04ffa3 to 2ffc615 Compare May 6, 2017 17:42

robfrawley added 2 commits May 6, 2017 14:05

remove deprecations and legacy code, new php and symfony requirements

38c2037

- use short array syntax - update composer requirements to PHP 7+ and Symfony 3+ - migrate fqcn strings to ::class compile-time constant - move travis operations to bash scripts

add upgrade notes on changes to the 2.x branch

2528fc2

robfrawley force-pushed the feature-deprecation-and-legacy-removal branch from 2ffc615 to 2528fc2 Compare May 6, 2017 18:06

robfrawley merged commit d9081a2 into liip:2.0 May 7, 2017

lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label May 7, 2017

robfrawley changed the title ~~[WIP] Remove deprecated and legacy code, migrate some syntax, cleanup~~ Remove deprecated and legacy code, migrate some syntax, cleanup May 7, 2017

rpkamp mentioned this pull request May 11, 2017

[Data Loader] [Data Locator] [Dependency Injection] Pass root paths to FileSystemLocator during construction #930

Merged

This was referenced May 20, 2017

HHVM and PHP7 differ on type annotations on internal functions (rtrim in particular) facebook/hhvm#7198

Closed

[RFC] What version of PHP are you running? What should we require? #873

Closed


		$container->setDefinition($resolverId, $resolverDefinition);
		$container->setDefinition($resolverId = 'liip_imagine.cache.resolver.'.$resolverName, $resolverDefinition);

		@@ -21,9 +22,9 @@ class SignerTest extends AbstractTest
		{
		public function testImplementsSignerInterface()

Remove deprecated and legacy code, migrate some syntax, cleanup #908

Remove deprecated and legacy code, migrate some syntax, cleanup #908

Conversation

robfrawley commented Apr 13, 2017 • edited by alexwilson Loading

Requirements

Todo Still

Should we Go Further

robfrawley commented Apr 13, 2017

alexwilson commented Apr 16, 2017

rpkamp commented Apr 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpkamp Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

robfrawley commented May 2, 2017 • edited Loading

robfrawley May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Koc commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley May 3, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley commented May 3, 2017

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

Choose a reason for hiding this comment

robfrawley commented May 5, 2017

alexwilson commented May 6, 2017

alexwilson left a comment

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

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

robfrawley commented May 6, 2017

robfrawley commented May 6, 2017 • edited Loading

rpkamp commented May 7, 2017

robfrawley commented May 7, 2017 • edited Loading

rpkamp commented May 7, 2017

robfrawley commented Apr 13, 2017 •

edited by alexwilson

Loading

rpkamp Apr 23, 2017 •

edited

Loading

robfrawley commented May 2, 2017 •

edited

Loading

robfrawley May 2, 2017 •

edited

Loading

robfrawley May 3, 2017 •

edited

Loading

robfrawley commented May 6, 2017 •

edited

Loading

robfrawley commented May 7, 2017 •

edited

Loading