use setFactory service definition method for symfony >= 2.6 (when possible) #566

adam187 · 2015-02-28T19:24:49Z

setFactoryClass, setFactoryMethod is deprecated since 2.6

lsmith77 · 2015-05-29T13:40:36Z

can you expand the .travis.yml file to include more versions of Symfony, if possible even including Symfony 3 https://github.com/doctrine/DoctrinePHPCRBundle/pull/187/files

adam187 · 2015-05-30T09:13:08Z

@lsmith77 I expanded symfony version up to 3.0.* in .travis.yml but not for 2.7 and up tests are failing on some other tests related to CacheResolver. I could probably fix them but i think it shouldn't be subject of this PR. Any suggestion?

gremo · 2015-05-31T09:29:38Z

Any updates on this? Deprecation messages are quite annoying :(

adam187 · 2015-05-31T10:13:33Z

@gremo You can use my fork until this one is resolved or generally disable deprecation messages with custom console file https://gist.github.com/adam187/9bdc986bcc69f4090f30

gremo · 2015-05-31T11:20:38Z

@adam187 thanks, I will use your fork until gets merged.

You should (I could be wrong) tag your current master version as 1.2.4 so that when this get merged we can simply remove the repository in composer.json and get the fixed version from the original repo (my minimum stability is stable):

"require": {
    "liip/imagine-bundle": "~1.0"
},
"repositories": [
    {
        "type": "vcs",
        "url": "https://github.com/adam187/LiipImagineBundle"
    }
]

Right now to use your fork I'm forced to use:

"require": {
    "liip/imagine-bundle": "dev-master"
}

adam187 · 2015-05-31T11:35:37Z

@gremo You have to use

"require": {
    "liip/imagine-bundle": "dev-set-factories"
}

because this changes hasn't been merged to master on my fork.
Also i think tagging not stable version on fork it's not a good idea.
It can create issues for syncing fork with upstream repo.

lsmith77 · 2015-06-01T08:26:33Z

to fix the memory issue with PHP 5.3 add the following to the travis build section:
if [[ "$TRAVIS_PHP_VERSION" != "hhvm" ]]; then echo "memory_limit = -1" >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini; fi

for the deprecation notices you can add the symfony phpunit bridge:
http://symfony.com/blog/new-in-symfony-2-7-phpunit-bridge

adam187 · 2015-06-02T08:35:56Z

@lsmith77 I added memory limit rule and phpunit-bridge but there are still some errors duo OptionsResolver/OptionsResolverInterface for symfony 2.7 and above.

Should i fix them here or is it a case for another PR ?

lsmith77 · 2015-06-02T09:24:03Z

no I think its good if we do it all in here. thank you for your work!

makasim · 2015-06-02T10:26:32Z

@adam187 Agree with @lsmith77 . The PR must be green before we can merge it.

makasim · 2015-06-02T10:27:20Z

DependencyInjection/Factory/Resolver/AwsS3ResolverFactory.php

        $awsS3ClientDefinition = new Definition('Aws\S3\S3Client');
-        $awsS3ClientDefinition->setFactoryClass('Aws\S3\S3Client');
-        $awsS3ClientDefinition->setFactoryMethod('factory');
+        if (method_exists($awsS3ClientDefinition, 'setFactory')) {


IMO, it is better to check symfony version here to be more explicit why we have this if.

And it will also reflect the preferred may and the one which will be removed in future.

@makasim I was referring how it was resolved in other bundles like https://github.com/doctrine/DoctrineBundle/blob/master/DependencyInjection/DoctrineExtension.php#L136 for example

okay, could you add a comment to else statement that it can be remove when we drop support of symfony <2.7

setFactoryClass, setFactoryMethod is deprecated since 2.6

to be more consistent optionResolover

… 2.6

adam187 · 2015-06-03T07:34:18Z

@lsmith77 Ready to check

makasim · 2015-06-03T07:45:55Z

DependencyInjection/LiipImagineExtension.php

+    /**
+     * @param ContainerBuilder $container
+     */
+    private function setFactories($container)


ContainerInterface typehint

adam187 · 2015-06-03T08:51:16Z

@makasim Updated

makasim · 2015-06-04T10:26:53Z

Imagine/Cache/Resolver/CacheResolver.php

+          'index_key' => 'string',
+        );
+
+        if (version_compare(Kernel::VERSION_ID, '20600') >= 0) {


What is the logic behind this change. Looks strange to me

@makasim Since symfony 2.6 setAllowedTypes api changed.

Now you call it with option and array of types instead off array where key is option and value is allowed types.

but it is a BC break, isn't it?

Yes, but it's deprecated since 2.6 to be removed in 3.0 https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/OptionsResolver/OptionsResolver.php#L591

adam187 · 2015-06-04T11:09:48Z

@makasim Updated

use setFactory service definition method for symfony >= 2.6 (when possible)

makasim · 2015-06-04T12:07:18Z

@adam187 tagged: https://github.com/liip/LiipImagineBundle/releases/tag/1.3.0

vishalmelmatti · 2015-06-05T06:57:50Z

Thanks for adding 2.7 support but still there is warning while updating composer.

Deprecated: Symfony\Component\DependencyInjection\Definition::setFactoryClass(Aws\S3\S3Client) is deprecated since version 2.6 and will be removed in 3.0. Use Definition::setFactory() instead. in vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Definition.php on line 101

makasim · 2015-06-05T07:48:23Z

@adam187 this deprecation was "fixed", wasn't it? @vishalmelmatti Did you upgrade to 1.3.0 version.

vishalmelmatti · 2015-06-05T07:52:50Z

@makasim yeah I upgraded to 1.3.0. This is my composer.lock

"name": "liip/imagine-bundle",
"version": "1.3.0",
"target-dir": "Liip/ImagineBundle",
"source": {
"type": "git",
"url": "https://github.com/liip/LiipImagineBundle.git",
"reference": "24c977ccf1a15854dc5fb3efc5de86011f8f0ef3"
},

adam187 · 2015-06-05T08:08:34Z

@makasim @vishalmelmatti Yes it was: https://github.com/liip/LiipImagineBundle/blob/master/DependencyInjection/Factory/Resolver/AwsS3ResolverFactory.php#L20-L21

vishalmelmatti · 2015-06-05T09:14:25Z

@adam187 @makasim Yeah I saw the code changes. But if you still try to run composer update or install it gives same warnings.

adam187 · 2015-06-05T09:26:29Z

@vishalmelmatti Are you sure this warning is from this bundle and not from some service definition in yml/xml?

vishalmelmatti · 2015-06-05T09:48:07Z

@adam187 Ah, it could be due to this http://symfony.com/doc/master/bundles/LiipImagineBundle/cache-resolver/aws_s3.html#create-resolver-as-a-service . I am defining my custom Aws client service.

adam187 · 2015-06-05T09:52:10Z

@vishalmelmatti

services:
    acme.amazon_s3:
        class: Aws\S3\S3Client
        factory: [Aws\S3\S3Client, factory]

vishalmelmatti · 2015-06-05T09:55:43Z

@adam187 Great. It clears warnings. Thanks a lot.

adam187 changed the title ~~use setFactory service definition method for symfony >= 2.6~~ use setFactory service definition method for symfony >= 2.6 (when possible) Feb 28, 2015

lsmith77 added the in progress label Mar 2, 2015

Koc mentioned this pull request May 26, 2015

make configuration compatible with sf3.0 #602

Closed

lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label May 29, 2015

adam187 force-pushed the set-factories branch 2 times, most recently from 48d00e4 to 0e3e544 Compare June 1, 2015 13:08

makasim reviewed Jun 2, 2015
View reviewed changes

adam187 added 13 commits June 3, 2015 08:29

use setFactory service definition method for symfony >= 2.6

f332505

setFactoryClass, setFactoryMethod is deprecated since 2.6

fix setting factory class

23425b5

expand symfony versions for testing

6d4e0c8

add testing for symfony 3.0

6c6ca23

cs fix

dc58bd9

add SYMFONY_DEPRECATIONS_HELPER=weak for symfony >= 2.7

491a9dd

add symfony 2.3 on php 5.5 for testing

6764a83

fix memory limit in travis tests

cf9d22d

add symfony/phpunit-bridge

80b99ea

add SYMFONY_DEPRECATIONS_HELPER as env var

8599396

use configureOptions and isDefined for symfony >= 2.6

61e2c1e

change setDefaultOptions to configureOptions

8d14b43

to be more consistent optionResolover

use new way of setting allowed types on OptionResolver for symfony >=…

2c2e235

… 2.6

add comment about DI fallback

658148d

adam187 force-pushed the set-factories branch from bcc0356 to 658148d Compare June 3, 2015 07:00

makasim reviewed Jun 3, 2015
View reviewed changes

adam187 added 4 commits June 3, 2015 09:50

add ContainerBuilder type hint

ce1ade3

add doc blocks

d7d06d5

split testing factories in to separate tests based on symfony version

4c577ac

extract testing service definition factories into separate tests

c217913

makasim mentioned this pull request Jun 4, 2015

Upgrading symfony to 2.7 displays warning #607

Closed

makasim reviewed Jun 4, 2015
View reviewed changes

split testing setDefaultOptions and configureOptions

0177d6f

makasim added a commit that referenced this pull request Jun 4, 2015

Merge pull request #566 from adam187/set-factories

24c977c

use setFactory service definition method for symfony >= 2.6 (when possible)

makasim merged commit 24c977c into liip:master Jun 4, 2015

lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Jun 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use setFactory service definition method for symfony >= 2.6 (when possible) #566

use setFactory service definition method for symfony >= 2.6 (when possible) #566

adam187 commented Feb 28, 2015

lsmith77 commented May 29, 2015

adam187 commented May 30, 2015

gremo commented May 31, 2015

adam187 commented May 31, 2015

gremo commented May 31, 2015

adam187 commented May 31, 2015

lsmith77 commented Jun 1, 2015

adam187 commented Jun 2, 2015

lsmith77 commented Jun 2, 2015

makasim commented Jun 2, 2015

makasim Jun 2, 2015

makasim Jun 2, 2015

adam187 Jun 2, 2015

makasim Jun 2, 2015

adam187 commented Jun 3, 2015

makasim Jun 3, 2015

adam187 commented Jun 3, 2015

makasim Jun 4, 2015

adam187 Jun 4, 2015

makasim Jun 4, 2015

makasim Jun 4, 2015

adam187 Jun 4, 2015

adam187 commented Jun 4, 2015

makasim commented Jun 4, 2015

vishalmelmatti commented Jun 5, 2015

makasim commented Jun 5, 2015

vishalmelmatti commented Jun 5, 2015

adam187 commented Jun 5, 2015

vishalmelmatti commented Jun 5, 2015

adam187 commented Jun 5, 2015

vishalmelmatti commented Jun 5, 2015

adam187 commented Jun 5, 2015

vishalmelmatti commented Jun 5, 2015

use setFactory service definition method for symfony >= 2.6 (when possible) #566

use setFactory service definition method for symfony >= 2.6 (when possible) #566

Conversation

adam187 commented Feb 28, 2015

lsmith77 commented May 29, 2015

adam187 commented May 30, 2015

gremo commented May 31, 2015

adam187 commented May 31, 2015

gremo commented May 31, 2015

adam187 commented May 31, 2015

lsmith77 commented Jun 1, 2015

adam187 commented Jun 2, 2015

lsmith77 commented Jun 2, 2015

makasim commented Jun 2, 2015

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

adam187 commented Jun 3, 2015

Choose a reason for hiding this comment

adam187 commented Jun 3, 2015

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

adam187 commented Jun 4, 2015

makasim commented Jun 4, 2015

vishalmelmatti commented Jun 5, 2015

makasim commented Jun 5, 2015

vishalmelmatti commented Jun 5, 2015

adam187 commented Jun 5, 2015

vishalmelmatti commented Jun 5, 2015

adam187 commented Jun 5, 2015

vishalmelmatti commented Jun 5, 2015

adam187 commented Jun 5, 2015

vishalmelmatti commented Jun 5, 2015