Skip to content

Fix EZP-24458: Refactor dynamic settings injection #1302

New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 18, 2015

Conversation

lolautruche
Copy link
Contributor

https://jira.ez.no/browse/EZP-24458

This PR removes usage of fake services to inject dynamic settings using factories.
Instead, usage of ExpressionLanguage component is introduced.

This allows to remove usage of a hack (fake services) and of a Symfony deprecated feature (synchronized services).

TODO

  • Refactor compiler passes
  • Deal with setter re-injection when config scope changes (e.g. content preview)
  • Fix tests

@lolautruche lolautruche force-pushed the EZP-24458_refactorDynamicSettings branch from 178ad53 to c7a00d6 Compare June 12, 2015 15:01
// This one will be transformed
$definition->addArgument( $dynamicSetting );
$resolverArguments[] = $this->createConfigResolverSubExpression( $this->parser->parseDynamicSetting( $dynamicSetting ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd wrap this.

@bdunogier
Copy link
Member

It looks really good so far. Yay.

@lolautruche lolautruche force-pushed the EZP-24458_refactorDynamicSettings branch from c7a00d6 to cefdcba Compare June 15, 2015 15:32
@lolautruche
Copy link
Contributor Author

Review ping @andrerom @bdunogier @pspanja @glye @yannickroger

@bdunogier
Copy link
Member

It still looks rather good to me.

@lolautruche lolautruche force-pushed the EZP-24458_refactorDynamicSettings branch 2 times, most recently from 330f2d6 to 8f231fa Compare June 16, 2015 09:43
@andrerom
Copy link
Contributor

+1, but as we need travis to run true tests please (cc @bdunogier) review the two PR's:
ezsystems/ezplatform#23
#1308

@lolautruche lolautruche force-pushed the EZP-24458_refactorDynamicSettings branch from 8f231fa to f8554f1 Compare June 17, 2015 07:36
@lolautruche
Copy link
Contributor Author

Rebased against master to have Travis run properly again.

@lolautruche
Copy link
Contributor Author

OK tests are now passing \o/

@pspanja
Copy link
Contributor

pspanja commented Jun 17, 2015

Looks good, +1

@@ -41,7 +42,7 @@ public function process( ContainerBuilder $container )
{
$dynamicSettingsServices = array();
$resettableServices = array();
$fakeServices = array();
$updateableServices = $container->getParameter( 'ezpublish.config_resolver.updeatable_services' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"updeatable" speling? Not sure if existing bug or introduced here.

@glye
Copy link
Member

glye commented Jun 17, 2015

+1 apart from nitpick above.

@bdunogier
Copy link
Member

+1

@lolautruche lolautruche force-pushed the EZP-24458_refactorDynamicSettings branch 2 times, most recently from b1063bd to b69e85c Compare June 17, 2015 12:07
@@ -21,7 +21,8 @@ class ConfigResolverParameterPassTest extends PHPUnit_Framework_TestCase
public function testProcess()
{
$container = new ContainerBuilder();
$dynamicSettingsServices = array();
$container->setParameter( 'ezpublish.config_resolver.updeatable_services', array() );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last updeatable :)

@lolautruche lolautruche force-pushed the EZP-24458_refactorDynamicSettings branch from b69e85c to e0568a1 Compare June 17, 2015 12:11
> https://jira.ez.no/browse/EZP-24458

This PR removes usage of *fake* services to inject dynamic settings using factories.
Instead, usage of ExpressionLanguage component is introduced.
@lolautruche lolautruche force-pushed the EZP-24458_refactorDynamicSettings branch from e0568a1 to 6e9cccc Compare June 17, 2015 14:19
lolautruche added a commit that referenced this pull request Jun 18, 2015
…ings

Fix EZP-24458: Refactor dynamic settings injection
@lolautruche lolautruche merged commit 6b73f1c into master Jun 18, 2015
@lolautruche lolautruche deleted the EZP-24458_refactorDynamicSettings branch June 18, 2015 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants