allow parameters in extension #189

docteurklein · 2013-09-13T20:08:00Z

This is a BC Break.

You must change phpspec.yml accordingly:

# before
extensions:
    - Some\Extension

# after:
extensions:
    Some\Extension: ~
    # or, and that's why
    Some\Other\Extension:
        option1: ~

I started to spec this until I realised this class is not speccable with current impl.

We should extract this extension loading mechanism to its own class.

docteurklein · 2013-09-13T20:11:57Z

Ha, funny, but I just realised you could register as much as DIC parameters as you want as soon as key is different from "extensions".

docteurklein · 2013-09-13T20:12:21Z

It doesn't mean we should not use this extension params thing.

ciaranmcnulty · 2013-09-13T20:28:51Z

You can add whatever Extension-specific config you want to the main YML, it doesn't have to be keyed via the extension name

docteurklein · 2013-09-13T21:10:00Z

@ciaranmcnulty thanks, I just noticed that later :/

Anyway, as said above, it does not mean we should not provide extension config under the extension class name.
It's better than having to maintain arbitrary DIC param names.

Thoughts @everzet ?

ciaranmcnulty · 2013-09-13T21:23:15Z

I'd agree if it didn't break BC.

MarcelloDuarte · 2013-09-13T21:34:54Z

@marcodebortoli would this impact MageSpec?

docteurklein · 2013-09-13T22:54:19Z

@MarcelloDuarte @marcodebortoli it does https://github.com/MageTest/MageSpec/blob/develop/src/MageTest/PhpSpec/MagentoExtension/Extension.php#L62 .

debo · 2013-09-14T13:26:53Z

@MarcelloDuarte what @docteurklein said but if you merge it I'll fix MageSpec straight away. I need to review some code anyway this weekend for the presentation so... feel free to proceed.

ciaranmcnulty · 2013-09-14T18:51:13Z

src/PhpSpec/Console/Application.php

@@ -404,7 +404,7 @@ protected function loadConfigurationFile(ServiceContainer $container)

        foreach ($config as $key => $val) {
            if ('extensions' === $key) {
-                foreach ($val as $class) {
+                foreach ($val as $class => $params) {
                    $extension = new $class;



Could you add a check here to see whether $class was numeric?

if (is_numeric($class)) { $class = $params; $params = array(); }

That would probably preserve BC

I don't think we need. There's only a handful of extensions, and we know must of the guys. Better to keep the code clean in this case, in my opinion. I will have a look at the expect helper (after jet leg has passed) and merge this.

@MarcelloDuarte But this BC break does not impact extension authors. It impacts extension users

instead of providing BC, we could at least throw an exception asking the user to update their config

+1

This will mean a user has to update all extensions associated with PhpSpec, then update their configurations. And it doesn't enable anything that wasn't possible before, just changes the syntax Extensions must use.

Actually, it introduces a new possibility: it decouples the YAML config from the DIC parameters, which means that extensions can make their config more semantic (using a nested array to group related settings), can validate the config provided for the extension and can provide BC for older formats when it gets refactored. Btw, these are the reason why Symfony2 bundles have a system for semantic configuration instead of relying on the user to set DIC parameters directly.

@stof has always the right words :)

@stof Extensions at present could access the YAML config, the question is whether extension-specific config should be under the extension key in the extensions: stanza, or whether extensions should be able to look at global config.

I would say that the extensions should be able to access both. They would have access to "extension-specific" config (semantic, simple for the user) AND to global container parameters.

@ciaranmcnulty Yes, extensions can access the YAML config. But if they want to provide a semantic configuration, which can be processed and for which it can provide BC, it means that you end with extra stuff in the DIC if you start by setting the semantic config as a parameter and then process it after that

MarcelloDuarte · 2013-12-20T12:21:09Z

@docteurklein I will tag 2.0 first and add this feature straight after, so existing extensions have time to adapt

docteurklein · 2013-12-20T13:02:58Z

perfect! Thanks.

ciaranmcnulty · 2014-02-03T20:41:31Z

@docteurklein I'm going to close this PR, can you please re-open against develop branch so we can merge in for 2.1?

docteurklein · 2014-08-29T12:12:38Z

since we have only one master branch anymore, can you reopen it ?

stof · 2014-08-29T12:13:23Z

@docteurklein can you rebase it ?

ciaranmcnulty · 2014-08-29T12:19:30Z

I don't think we can break the old format, can you handle both? Maybe by detecting when the value is an array, or detecting when the key is numeric (we're never going to have a numeric Extension name)

docteurklein · 2014-08-29T12:32:50Z

i'll do that, and rebase.

ciaranmcnulty · 2014-08-29T12:37:51Z

Thanks - I know we discussed breaking BC before, but there are more extensions now than there used to be :-)

docteurklein · 2014-08-29T12:46:31Z

src/PhpSpec/Console/Application.php

-                foreach ($val as $class) {
-                    $extension = new $class();
+                foreach ($val as $extensionKey => $extensionValue) {
+                    $extension = is_integer($extensionKey) ? new $extensionValue : new $extensionKey;


not very elegant, but if you have better I'll take it!

norberttech · 2014-08-29T13:11:47Z

@docteurklein what do you think about passing optional parameters as a extension __construtor arguments? I have very similar extension system in my project and this is how I construct extensions: https://github.com/coduo/TuTu/blob/master/src/Coduo/TuTu/Extension/Initializer.php

stof · 2014-08-29T13:13:21Z

src/PhpSpec/Console/Application.php

+                        $class = $extensionConfig;
+                        $extensionConfig = array();
+                    }
+                    var_dump($class);


needs to be removed

docteurklein · 2014-08-29T13:45:14Z

done. What do you think ?

shanethehat · 2014-08-29T23:05:23Z

I like that future versions of Magespec could have the extension-specific config placed in a clear place in the config file. The proposed BC break could be adopted very quickly, and with a small amount of warning we could be compatible as soon as this merges and tags. @marcodebortoli and @jamescowie?

ciaranmcnulty · 2014-08-31T15:04:54Z

src/PhpSpec/Console/Application.php

-                    $extension = new $class();
+                foreach ($val as $class => $extensionConfig) {
+                    if (is_integer($class)) {
+                        $class = $extensionConfig;


You'll need to validate that $class is a string here or get a weird error a few lines later

ciaranmcnulty · 2014-11-24T12:11:14Z

@docteurklein Can you address my notes above?

docteurklein · 2014-11-24T12:30:35Z

rabased.
@ciaranmcnulty sorry for such a long time, i simply am so busy :)
Anyway, here are the changes concerning the class name validation.

docteurklein · 2014-11-24T12:31:35Z

I didn't implement the BC ParametrizedExtension, since I'm not sure this is something we want.

ciaranmcnulty · 2014-11-29T16:57:21Z

BC is a big issue - it would be much better if you can preserve it.

docteurklein · 2014-12-01T12:31:38Z

@ciaranmcnulty i finally introduced a ParametrizedExtensionInterface.
Extensions creators who want to allow extension parameters will have to implement this interface instead of the default one.

docteurklein · 2014-12-01T13:10:32Z

and I got a D for that! (which I understand, seeing the code :) )

ciaranmcnulty · 2014-12-01T13:16:50Z

Optional arguments are often missing from Scrutinizer reports - report it as an error or do a PR to php-analyser

docteurklein · 2014-12-02T09:05:14Z

ho crap: https://travis-ci.org/phpspec/phpspec/jobs/42623205#L2013

5.3.3 is definitly a bummer :)
What do you think sould I do ?

ciaranmcnulty · 2014-12-02T09:08:31Z

Upvote this #571 ;-P

docteurklein · 2014-12-02T09:13:54Z

that's exactly what I wanted to hear :)

ciaranmcnulty · 2015-01-18T19:46:41Z

@docteurklein I don't think we'll be updating the PHP version soon

stof · 2015-01-18T19:52:25Z

@ciaranmcnulty I suggest putting this on hold until we decide what we do with Testwork (if we go the way of using Testwork extensions, this proposal will be obsolete)

docteurklein · 2015-01-19T08:14:05Z

@stof @ciaranmcnulty good idea.

ciaranmcnulty · 2015-02-09T21:37:58Z

I'm going to close the PR for now, it doesn't mean it's not a good idea!

docteurklein referenced this pull request in docteurklein/Symfony2Extension Sep 13, 2013

refactor extension to allow params

64b7da5

ciaranmcnulty reviewed Sep 14, 2013
View reviewed changes

docteurklein referenced this pull request in docteurklein/Symfony2Extension Sep 17, 2013

do not use extension config yet

05ebcd7

ciaranmcnulty closed this Feb 3, 2014

docteurklein mentioned this pull request Aug 29, 2014

Add extensions cookbook #485

Merged

ciaranmcnulty reopened this Aug 29, 2014

docteurklein force-pushed the feature/extension-params branch from 57b4230 to 452a587 Compare August 29, 2014 12:44

docteurklein reviewed Aug 29, 2014
View reviewed changes

docteurklein force-pushed the feature/extension-params branch from 452a587 to 1c2ec7a Compare August 29, 2014 13:12

stof reviewed Aug 29, 2014
View reviewed changes

docteurklein force-pushed the feature/extension-params branch from 1c2ec7a to 31e2470 Compare August 29, 2014 13:14

docteurklein force-pushed the feature/extension-params branch from dc0d473 to a96f67e Compare August 30, 2014 07:54

ciaranmcnulty added the feature label Aug 31, 2014

ciaranmcnulty reviewed Aug 31, 2014
View reviewed changes

ciaranmcnulty removed this from the 2.1 milestone Nov 24, 2014

docteurklein force-pushed the feature/extension-params branch from a96f67e to 16309a2 Compare November 24, 2014 12:29

allow parameters in extension

6b1edd1

docteurklein force-pushed the feature/extension-params branch from 16309a2 to 6b1edd1 Compare December 1, 2014 12:30

ciaranmcnulty added this to the 2.2 milestone Dec 2, 2014

ciaranmcnulty mentioned this pull request Dec 2, 2014

[WIP] Introduce symfony service container and testwork extensions #575

Closed

ciaranmcnulty closed this Feb 9, 2015

This was referenced Jun 17, 2016

Registered configured custom matchers #949

Merged

[WIP] Wrapped PhpSpec config in a phpspec keyword #955

Closed

Scope extension parameters to relevant extension #958

Merged

allow parameters in extension #189

allow parameters in extension #189

Conversation

docteurklein commented Sep 13, 2013

docteurklein commented Sep 13, 2013

docteurklein commented Sep 13, 2013

ciaranmcnulty commented Sep 13, 2013

docteurklein commented Sep 13, 2013

ciaranmcnulty commented Sep 13, 2013

MarcelloDuarte commented Sep 13, 2013

docteurklein commented Sep 13, 2013

debo commented Sep 14, 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

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

MarcelloDuarte commented Dec 20, 2013

docteurklein commented Dec 20, 2013

ciaranmcnulty commented Feb 3, 2014

docteurklein commented Aug 29, 2014

stof commented Aug 29, 2014

ciaranmcnulty commented Aug 29, 2014

docteurklein commented Aug 29, 2014

ciaranmcnulty commented Aug 29, 2014

Choose a reason for hiding this comment

norberttech commented Aug 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

docteurklein commented Aug 29, 2014

shanethehat commented Aug 29, 2014

Choose a reason for hiding this comment

ciaranmcnulty commented Nov 24, 2014

docteurklein commented Nov 24, 2014

docteurklein commented Nov 24, 2014

ciaranmcnulty commented Nov 29, 2014

docteurklein commented Dec 1, 2014

docteurklein commented Dec 1, 2014

ciaranmcnulty commented Dec 1, 2014

docteurklein commented Dec 2, 2014

ciaranmcnulty commented Dec 2, 2014

docteurklein commented Dec 2, 2014

ciaranmcnulty commented Jan 18, 2015

stof commented Jan 18, 2015

docteurklein commented Jan 19, 2015

ciaranmcnulty commented Feb 9, 2015