Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI][DX] Allow exclude to be an array of patterns#24428
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $loader->registerClasses( | ||
| newDefinition(), | ||
| 'Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\\', | ||
| 'Prototype/*',array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FileLoader::registerClasses docblock must be updated, right?
| * @param string $exclude A globed path of files to exclude |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're absolutely right, I completely missed this one. My bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Docblock updated
| * @param Definition$prototype A definition to use as template | ||
| * @param string$namespace The namespace prefix of classes in the scanned directory | ||
| * @param string$resource The directory to look for classes, glob-patterns allowed | ||
| * @param string|array $excludeA globed path of files toexcludeor an array ofglobedpaths of files to exclude |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should it bestring[] instad ofarray ?
| if (null ===$excludePrefix) { | ||
| $excludePrefix =$resource->getPrefix(); | ||
| if ($excludePatterns) { | ||
| $excludePatterns = (array)$parameterBag->unescapeValue($parameterBag->resolveValue($excludePatterns)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd rather do the cast inregisterClasses and add thearray $excludePatterns typehint in this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's done
nicolas-grekas commentedOct 5, 2017
This might be enough for Yaml, but this doesn't benefit Xml nor the PHP configurators yet, do we want to make this available for these formats also? |
nicolas-grekas commentedOct 8, 2017
Moving to 4.1. |
apetitpa commentedOct 8, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas I'm sorry I could not finish this earlier, I'll rebase and edit base branch on github |
nicolas-grekas commentedDec 29, 2017
@apetitpa let's continue this one? |
apetitpa commentedDec 30, 2017
@nicolas-grekas I'm back on it ! Would you mind giving me a hint on what's left to do please ? |
nicolas-grekas commentedDec 30, 2017
I think we should make this work somehow for other config formats (xml and php-dsl) |
apetitpa commentedJan 9, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas Thanks for your help I have rebased on master and started to work something out, so this means I should make this piece of xml configuration work, right ? <prototypenamespace="AppBundle\"resource="../../src/AppBundle/*"> <exclude>../../src/AppBundle/{Entity,Repository}</exclude></prototype> About the php based configuration it should already work since you can now pass an array as the third argument of $this->registerClasses($definition,'AppBundle\\','../../src/AppBundle/*',array('../../src/AppBundle/{Entity,Repository}')); |
nicolas-grekas commentedJan 10, 2018
I was more specifically referring to the Configurator in the Loader namespace. Look for a call to this method there, you should find something to adjust. |
nicolas-grekas commentedFeb 11, 2018
ping@apetitpa |
nicolas-grekas commentedMar 22, 2018
@apetitpa do you think you can finish this PR before the feature freeze by the end of the month? |
apetitpa commentedMar 27, 2018
@nicolas-grekas I'll do my best but I'm very short in time these days. I'll give it a try probably on next saturday evening, is it too late ? |
stof left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Whne updating the XML loader, don't forget to update the XSD too.
| * @param Definition$prototype A definition to use as template | ||
| * @param string$namespace The namespace prefix of classes in the scanned directory | ||
| * @param string$resource The directory to look for classes, glob-patterns allowed | ||
| * @param string|string[] $excludeA globed path of files toexcludeor an array ofglobedpaths of files to exclude |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
right type should actually bestring|string[]|null (the phpdoc should bestring|null in 3.4)
| foreach ($this->glob($excludePattern,true,$resource)as$path =>$info) { | ||
| if (null ===$excludePrefix) { | ||
| $excludePrefix =$resource->getPrefix(); | ||
| if ($excludePatterns) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you could remove this diff, handling the empty array like any other array (thestring|null type was needing a dedicated handling ofnull before)
magnetik commentedApr 27, 2018
It's now too late for 4.1. If you miss time to finish@apetitpa I can take it over if it's good for you. |
magnetik commentedApr 27, 2018
I've created a pull request (just above) to finish it :) |
nicolas-grekas commentedApr 27, 2018
apetitpa commentedApr 30, 2018
I'm sorry I couldn't make it guys and thank you@magnetik for taking over. |
…netik)This PR was merged into the 4.2-dev branch.Discussion----------[DI][DX] Allow exclude to be an array of patterns| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#23956| License | MITThis is basically continuing#24428.In YAML before:```yamlAppBundle\: resource: '../../src/AppBundle/*' exclude: '../../src/AppBundle/{Entity,Payload,Repository}'```in YAML after:```yamlAppBundle\: resource: '../../src/AppBundle/*' exclude: - '../../src/AppBundle/{Entity,Payload,Repository}' - '../../src/AppBundle/Event/*Event.php'```In XML before:```xml<prototype namespace="App\" resource="../src/*" exclude="../src/{Entity,Migrations,Tests}" />```in XML after:```xml<prototype namespace="App\" resource="../src/*"> <exclude>../src/{Entity,Migrations,Tests}</exclude> <exclude>../src/Yolo</exclude></prototype>```In PHP before:```php$di->load(Prototype::class.'\\', '../Prototype') ->autoconfigure() ->exclude('../Prototype/{OtherDir,BadClasses}')```In PHP after:```php $di->load(Prototype::class.'\\', '../Prototype') ->autoconfigure() ->exclude(['../Prototype/OtherDir', '../Prototype/BadClasses'])```Everything is backward compatible.Maybe a decision about handling both attribute exclude and element exclude in XML should be taken.Commits-------3ae3a03 [DI][DX] Allow exclude to be an array of patterns (taking#24428 over)
Uh oh!
There was an error while loading.Please reload this page.
Before:
After: