Add support for Puli #995
Add support for Puli #995
Conversation
|
Something on what could be improved on the "extension detection" would be to have a real system of extension locators (one for class locator, one for file locator, one for puli locator, ... and so on, the first "supports" win), but I am not too sure if this wouldn't be too overkill ? |
|
|
||
| // feed extension on first call | ||
| if (null === $extensions) { | ||
| $extensions = []; |
There was a problem hiding this comment.
I have to change this into array(), I know.
|
I'd rather not tie |
Test this by writing a Behat scenario doing it with a custom extension. |
Yup, but I need to be sure that puli is available, that's the main pain IMO. Maybe add a tag or something ? I don't really want to add it in the |
@Taluu why not? |
| @@ -228,4 +234,29 @@ private function validateExtensionInstance($extension, $locator) | |||
| ), $locator); | |||
| } | |||
| } | |||
|
|
|||
| private function discoverExtensionWithPuli($locator) | |||
There was a problem hiding this comment.
I'd rather this method and all the other mentions of Puli to be hidden behind an abstraction.
There was a problem hiding this comment.
You mean abstracting external tools or abstracting the loaders (one for the file loader, one for class locator, one for puli, ... and so on) ?
|
@everzet Because I feel like it should be more as a suggestion or optionnal dependency rather than required dev deps, but if you say it's okay, then I don't really mind |
|
@Taluu dev requirements are things needed to run our tests. They have no impact on our users. So we can put what we need in them. |
| @@ -53,5 +54,8 @@ | |||
| } | |||
| }, | |||
|
|
|||
| "prefer-stable": true, | |||
| "minimum-stability": "dev", | |||
There was a problem hiding this comment.
Had to it because puli/composer-plugin is still in beta (composer doesn't accept to install it then, as it is "not stable" yet)
There was a problem hiding this comment.
I think this is the bit that breaks the build on 5.3 :/
There was a problem hiding this comment.
I feel uneasy about doing this with stable core release. If that's the only way of doing the feature, my preference shifts to just providing an extension point in the core, but Puli being an extension (maybe even official).
There was a problem hiding this comment.
So just splitting the extension loader ? Can do
There was a problem hiding this comment.
Yeah, well, the only challenge you will have now is that you will need to introduce an extension point for extension instantiators, which is definitely a head-scratcher.
There was a problem hiding this comment.
Well, first things first...
Or what can be done, as it is done in the travis configuration, it's to enable these only in travis (or in dev through a global configuration ?). So it would mean to remove these two configuration options. Don't forget that it's only for dev-mode...
There was a problem hiding this comment.
But it doesn't solve the problem that if Puli is provided in an extension (official or not), it would indeed be hard to solve the problem of extending these (you would actually need to load the extension through a normal way ?)
Or what could be done, instead of relying on these, would be to support something like a composer package type, which would be loaded automatically ?
| * | ||
| * @param null|string $path | ||
| */ | ||
| public function setExtensionsPath($path) |
There was a problem hiding this comment.
this wasn't used, but as it is public, I'm wondering if this could be considered a BC break ; but I didn't find a way on how to keep compatibility though...
Taluu
force-pushed
the
puli
branch
2 times, most recently
from
5c4e722
to
7976815
Compare
|
@everzet PR updated with an abstraction on the initializers. The tests are not passing on php 5.3 for reasons that are eluding me (I can't manage to find the "line 446" on the puli plugin...). Tests are missing but I'll add these later. |
Taluu
force-pushed
the
puli
branch
3 times, most recently
from
32d222d
to
9173343
Compare
Taluu
force-pushed
the
puli
branch
3 times, most recently
from
e25402f
to
7055243
Compare
|
I just reran the test and it still fails. Something is broken |
|
Mmh, on travis it seems it is installing the |
|
BTW, wasn't 5.3 support dropped ? Why is it still on travis ? |
Taluu
force-pushed
the
puli
branch
4 times, most recently
from
6d15861
to
f938f4d
Compare
|
Closing this one, as this could be in a separate (official or not) extension in favor of #1018. Even though we first need to solve the "head-scratching issue" mentioned by @everzet |
this implements what is described in #989, although it is right now pretty specific for puli (as there is no "standards" currently).
I am not sure how to test this, but maybe registering the built-in extensions in the puli json file, and then remove the
extensionsparameter from the constructor could lead to something ? And then test that a built-in extension loads itself correctly ?