Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DI][DX] Allow exclude to be an array of patterns#27075

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

Merged
fabpot merged 1 commit intosymfony:masterfrommagnetik:exclude-array
May 9, 2018

Conversation

@magnetik
Copy link
Contributor

@magnetikmagnetik commentedApr 27, 2018•
edited by stof
Loading

QA
Branch?4.2
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#23956
LicenseMIT

This is basically continuing#24428.

In YAML before:

AppBundle\:resource:'../../src/AppBundle/*'exclude:'../../src/AppBundle/{Entity,Payload,Repository}'

in YAML after:

AppBundle\:resource:'../../src/AppBundle/*'exclude:    -'../../src/AppBundle/{Entity,Payload,Repository}'    -'../../src/AppBundle/Event/*Event.php'

In XML before:

<prototypenamespace="App\"resource="../src/*"exclude="../src/{Entity,Migrations,Tests}" />

in XML after:

<prototypenamespace="App\"resource="../src/*">  <exclude>../src/{Entity,Migrations,Tests}</exclude>  <exclude>../src/Yolo</exclude></prototype>

In PHP before:

$di->load(Prototype::class.'\\','../Prototype')        ->autoconfigure()        ->exclude('../Prototype/{OtherDir,BadClasses}')

In PHP after:

$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.

TomasVotruba, demonkoryu, mdubowski, BoShurik, tdomarkas, einenlum, mykiwi, nesk, behnoushnorouzi, zmitic, and 3 more reacted with thumbs up emojiarderyp, dmaicher, apetitpa, TomasVotruba, Koc, and einenlum reacted with hooray emojiKocal, nesk, and vudaltsov reacted with heart emoji
@carsonbotcarsonbot added Status: Needs Review DependencyInjection DXDX = Developer eXperience (anything that improves the experience of using Symfony) Feature Deprecation labelsApr 27, 2018
@magnetikmagnetikforce-pushed theexclude-array branch 2 times, most recently froma879aad to029b553CompareApril 27, 2018 13:59
@linaori
Copy link
Contributor

Ah nice, this will make life a lot easier for complex exclusion rules in existing codebases!

TomasVotruba reacted with thumbs up emoji

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

looks great, thanks for taking over!

if (null !==$definition =$this->parseDefinition($service,$file,$defaults)) {
if ('prototype' ===$service->tagName) {
$this->registerClasses($definition, (string)$service->getAttribute('namespace'), (string)$service->getAttribute('resource'), (string)$service->getAttribute('exclude'));
$excludes =array_map(function ($node) {return$node->nodeValue; },$this->getChildren($service,'exclude'));

Choose a reason for hiding this comment

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

would array_column work here instead of array_map?

Copy link
Member

Choose a reason for hiding this comment

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

nope. We need a property access, not an array access

if ($service->hasAttribute('exclude')) {
$excludes[] = (string)$service->getAttribute('exclude');
}
$this->registerClasses(

Choose a reason for hiding this comment

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

should be kept on one line - it would better fit our CS IMHO

$definition,
(string)$service->getAttribute('namespace'),
(string)$service->getAttribute('resource'),
(array)$excludes

Choose a reason for hiding this comment

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

it's already an array, no need to cast I suppose

if ('prototype' ===$service->tagName) {
$this->registerClasses($definition, (string)$service->getAttribute('namespace'), (string)$service->getAttribute('resource'), (string)$service->getAttribute('exclude'));
$excludes =array_map(function ($node) {return$node->nodeValue; },$this->getChildren($service,'exclude'));
if ($service->hasAttribute('exclude')) {

Choose a reason for hiding this comment

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

I would allow only tags XOR attribute (like done in similar cases in the loader AFAIK

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

So in cas both are present, should I throw an exception?

Copy link
Member

Choose a reason for hiding this comment

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

I would say yes

@TomasVotruba
Copy link
Contributor

This is awesome, great job!

@magnetikmagnetikforce-pushed theexclude-array branch 2 times, most recently from3a287e2 to246640fCompareMay 2, 2018 12:14
@magnetik
Copy link
ContributorAuthor

I've updated the PR according to the changes. I'll do a PR for symfony doc too if needed.

$excludes =array_column($this->getChildren($service,'exclude'),'nodeValue');
if ($service->hasAttribute('exclude')) {
if (count($excludes) >0) {
thrownewInvalidArgumentException('You cannot use both attribute "exclude" and <exclude> tags at the same time.');
Copy link
Member

Choose a reason for hiding this comment

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

both the "exclude" attribute and ...

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

done

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(would be OK on 4.1 on my side, the topic has been on the table for a few months already).

@nicolas-grekasnicolas-grekas changed the title[DI][DX] Allow exclude to be an array of patterns (taking #24428 over)[DI][DX] Allow exclude to be an array of patternsMay 4, 2018
Copy link
Contributor

@ogizanagiogizanagi left a comment

Choose a reason for hiding this comment

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

Same, would be great to have this in 4.1. We already missed the opportunity to have it in 3.4/4.0.
Thanks for taking over@magnetik .

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

👍 for 4.1

* @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[]|null $excludeA globed path of files toexcludeor an array ofglobedpaths of files to exclude
Copy link
Member

Choose a reason for hiding this comment

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

globbed? (not sure but I think so)

Copy link
ContributorAuthor

@magnetikmagnetikMay 9, 2018•
edited
Loading

Choose a reason for hiding this comment

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

done (both occurrences)

@fabpot
Copy link
Member

Thank you@magnetik.

VasekPurchart, magnetik, arderyp, and atakchidi reacted with hooray emoji

@fabpotfabpot merged commit3ae3a03 intosymfony:masterMay 9, 2018
fabpot added a commit that referenced this pull requestMay 9, 2018
…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)
@nicolas-grekasnicolas-grekas modified the milestones:next,4.1Nov 1, 2018
This was referencedNov 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

+1 more reviewer

@ogizanagiogizanagiogizanagi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

DependencyInjectionDXDX = Developer eXperience (anything that improves the experience of using Symfony)FeatureStatus: Reviewed

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

9 participants

@magnetik@linaori@TomasVotruba@fabpot@nicolas-grekas@stof@ogizanagi@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp