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] Fixing missing "exclude" functionality from PSR4 loader#22680

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:masterfromweaverryan:allow-psr4-exclude
May 14, 2017

Conversation

@weaverryan
Copy link
Member

@weaverryanweaverryan commentedMay 9, 2017
edited
Loading

QA
Branch?master
Bug fix?yes
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsnone
LicenseMIT
Doc PRTODO

When the PSR4 loader was added in#21289,@nicolas-grekas said this:

given that glob() is powerful enough to include/exclude dirs, I removed the Test special exclusion (source:#21289 (comment))

But, I don't believe that's true!Glob is all about inclusion, not exclusion - the maximum you can exclude is a single character. Thus, I've marked this as a bug.

My motivation came from upgrading KnpU to the new 3.3 DI stuff. We have many directories (40) insrc/, and listing them all one-by-one inresource: is crazy - theresource line would be ~350 characters long and quite unreadable. What I really want to do is includeeverything and then exclude few directories. I tried to do this withglob, but it's not possible.

This PR allows for something like this:

services:_defaults:public:false# ...AppBundle\:resource:'../../src/AppBundle/*'exclude:'../../src/AppBundle/{AppBundle.php,Entity}'

This worksbeautifully in practice. And even if I forget to exclude a directory - since the services are private -they're ultimately removed from the container anyways. In fact, theonly reason I need to excludeanything is because of the new "service argument resolver", which causes entities to not be properly removed from the compiled container.

Thanks!

Pierstoval, apetitpa, robfrawley, TomasVotruba, yceruto, and theofidry reacted with thumbs up emoji
@nicolas-grekas
Copy link
Member

You don't have to use a single psr4 block. You can have several, one per sub namespace, no? This would still be less config lines than 3.2-style config, no? Thus, not sure about the "bug fix" part. Registering everything as a service by default looks strange.

@stof
Copy link
Member

@nicolas-grekas if you create lots of different subnamespaces to organize your code, most of these subnamespaces will be about services (as you will keep the entities grouped somewhere to avoid making your Doctrine ORM config totally crazy).
So this looks like a sensible case IMO.

Adding 1 PSR-4 rule per subnamespace still requires listing them all one-by-one (and remembering to add the new subnamespace in the list when adding one)

@nicolas-grekas
Copy link
Member

Looking atsymfony/symfony-standard#1070, I think like it. I'm mostly challenging the bug fix nature of the PR. Maybe I shouldn't :)

@nicolas-grekas
Copy link
Member

Should the pattern be matched against the path or the class? That's not obvious to me.

@weaverryan
Copy link
MemberAuthor

Should the pattern be matched against the path or the class? That's not obvious to me.

Theexclude is against the file path (not the class) to be consistent withresource (and to allow you to skip a randomfunctions.php file that may not include any classes). Hopefully the default example insymfony/symfony-standard#1070 will make this obvious. Or we could rename toexclude_paths (or some other idea?).

I'm mostly challenging the bug fix nature of the PR. Maybe I shouldn't :)

I chose bug in part because I think this is important for 3.3 :). Well, specifically, I thinksymfony/symfony-standard#1070 is important for 3.3, and this is needed. But also, if you have 40 directories, listing them all one-by-onejust to avoidEntity really sucks :).

$loader =newTestFileLoader($container,newFileLocator(self::$fixturesPath.'/Fixtures'));

$loader->registerClasses(newDefinition(),'Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\\','Prototype/%sub_dir%/*');
$loader->registerClasses(newDefinition(),'Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\\','Prototype/%sub_dir%/*','');
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed, right?

newDefinition(),
'Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\\',
'Prototype/*',
// load everything, except for OtherDir/AnotherSub & Foo.ph
Copy link
Contributor

Choose a reason for hiding this comment

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

.php and I would remove thefor

@weaverryanweaverryanforce-pushed theallow-psr4-exclude branch 2 times, most recently from608b317 tob08065eCompareMay 10, 2017 18:23
@javiereguiluzjaviereguiluz added this to the3.3 milestoneMay 11, 2017
@javiereguiluz
Copy link
Member

javiereguiluz commentedMay 11, 2017
edited
Loading

Nice feature ... but the proposed syntax looks confusing:

AppBundle\:    resource: '../../src/*'    exclude: '^Entity'

The^Entity config means"not Entity/", so"exclude ^Entity/" looks like a double negation and I understand it as "exclude not Entity/" ... so "exclude everything except Entity/".

If the option name is already "exclude", why can't we use positive glob expressions for it?

@jvasseur
Copy link
Contributor

@javiereguiluz The exclude option is a regex, not a glob so^ means start of the string.

Maybe the proposition should be updated to use a glob for the exclude part to, it's strange to use a glob for the resource key and a regex for the exclude key.

apetitpa reacted with thumbs up emoji

@javiereguiluz
Copy link
Member

@jvasseur thanks! So I was confused ... but for the wrong reasons 😵 I agree that this should be a glob, just like the resource key.

@GuilhemN
Copy link
Contributor

GuilhemN commentedMay 11, 2017
edited
Loading

You're lacking| using glob, which is used insymfony/symfony-standard#1070.
Allowing an array could be a solution but would not be consistent in XML (resource an attribute,exclude a tag...).
We could also extend the glob syntax and support| but I'm not sure this would be accepted...

Do you have another idea?

@jvasseur
Copy link
Contributor

jvasseur commentedMay 11, 2017
edited
Loading

@GuilhemN glob syntax has an equivalent to|:{,}.

symfony/symfony-standard#1070 would become{AppBundle.php,Entity,Repository} using a glob. (or maybe../../src/{AppBundle.php,Entity,Repository} to have a path relative to the current file).

GuilhemN reacted with thumbs up emoji

@GuilhemN
Copy link
Contributor

Right, I didn't think about this possibility!
👍 for using glob then.

@fabpot
Copy link
Member

fabpot commentedMay 11, 2017
edited
Loading

@weaverryan For consistency, I'm also for using globs instead of regexes.

robfrawley and TomasVotruba reacted with thumbs up emoji

@weaverryanweaverryanforce-pushed theallow-psr4-exclude branch 2 times, most recently from2016fe3 to4f43922CompareMay 12, 2017 17:35
@weaverryan
Copy link
MemberAuthor

Updated to use glob! Great idea (seems obvious in hindsight). PR example updated to show its usage. Tests are passing - the one failure is also failing on master.

I used the full path for theexclude, but if we like it better, we should be able to make it relative:

resource:'../../src/AppBundle/*'# currentexclude:'../../src/AppBundle/{AppBundle.php,Entity}'# but we could possibly shorten if we wantexclude:{AppBundle.php,Entity}`

What we have now is a bit longer... but it also looks really clear.

yceruto reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

100% agree with the long version. We should throw an exception when "exclude" does not share the same prefix than with "resource"

@weaverryan
Copy link
MemberAuthor

Ok, long version it is then! I added a 2nd commit that throws an exception if theexclude prefix is not a sub-set of theresource prefix.

Anything else? Also, this PR will fix a "bug" / "bad error" caused by autowiring in some cases (symfony/symfony-standard#1070 (comment)).

}

// realpath to make sure Windows slashes are consistent
$excludePaths[realpath($path)] =true;
Copy link
Member

Choose a reason for hiding this comment

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

realpath() with returnfalse if the $path does not exist. You should take care of that. We try to avoid usingrealpath() in Symfony (another issue is when you are using a phar -- not sure we need to support that case anyway).

Choose a reason for hiding this comment

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

the realpath also won't work inside a phar

Choose a reason for hiding this comment

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

realpath($path) ?: $path

}
}

if (isset($excludePaths[realpath($path)])) {

Choose a reason for hiding this comment

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

Same here of course

@weaverryan
Copy link
MemberAuthor

I've removedrealpath entirely and simply replaced windows\ with/.

Theresource paths andexclude paths simply need to be consistent, so we can match them up. They should already be consistent in most cases, but based on where you put your wildcard, you could have paths inexclude whose slashes don'tquite match up with theresource slashes. Flipping all the slashes for purposes of matching should work beautifully.

So, things should be good now!

$excludePrefix =$resource->getPrefix();
}

// realpath to make sure Windows slashes are consistent

Choose a reason for hiding this comment

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

Comment needs a fix

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.

👍

@fabpot
Copy link
Member

Thank you@weaverryan.

@fabpotfabpot merged commit7d07f19 intosymfony:masterMay 14, 2017
fabpot added a commit that referenced this pull requestMay 14, 2017
…loader (weaverryan)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Fixing missing "exclude" functionality from PSR4 loader| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | none| License       | MIT| Doc PR        | TODOWhen the PSR4 loader was added in#21289,@nicolas-grekas said this:> given that glob() is powerful enough to include/exclude dirs, I removed the Test special exclusion (source:#21289 (comment))But, I don't believe that's true! [Glob is all about inclusion, not exclusion](https://en.wikipedia.org/wiki/Glob_(programming)#Syntax) - the maximum you can exclude is a single character. Thus, I've marked this as a bug.My motivation came from upgrading KnpU to the new 3.3 DI stuff. We have many directories (40) in `src/`, and listing them all one-by-one in `resource:` is crazy - the `resource` line would be ~350 characters long and quite unreadable. What I really want to do is include *everything* and then exclude few directories. I tried to do this with `glob`, but it's not possible.This PR allows for something like this:```ymlservices:    _defaults:        public: false    # ...    AppBundle\:        resource: '../../src/AppBundle/*'        exclude: '../../src/AppBundle/{AppBundle.php,Entity}'```This works *beautifully* in practice. And even if I forget to exclude a directory - since the services are private -  **they're ultimately removed from the container anyways**. In fact, the *only* reason I need to exclude *anything* is because of the new "service argument resolver", which causes entities to not be properly removed from the compiled container.Thanks!Commits-------7d07f19 Allowing prototype/PSR4 service loading ability to exclude, because glob doesn't support this
fabpot added a commit to symfony/symfony-standard that referenced this pull requestMay 14, 2017
…ices (weaverryan)This PR was merged into the 3.3-dev branch.Discussion----------Making *all* classes in src/AppBundle available as servicesHi guys!tl;dr; This makes *all* services from `src/AppBundle/` available as services by default.> This PR depends onsymfony/symfony#22680 andsymfony/symfony#22665If this seems crazy to you, it's actually not: it's almost exactly how the system already works :).By registering *all* of `src/AppBundle`, we will almost undoubtedly register some classes that should *not* be services. But, because the services are private, unless you actually reference them somewhere, they're simply removed. In other words, **we're not *really* importing all services from `src/AppBundle`, we're making all classes in `src/AppBundle` *available* to be used as services**.Today, thanks to autowiring, if you type-hint a class that is *not* registered as a service, autowiring registers it for you. That means that **the total number of services in the compiled container today versus after this change will be the same**.* **Today** if you don't explicitly register a service as public and don't type-hint/reference it anywhere, it's never added.* **After**     if you don't explicitly register a service as public and don't type-hint/reference it somewhere, it's removed from the final containerSo, the end result is the same as now. And there are a number of advantages:**PROS*** A) **Better developer experience: when a dev creates a new directory**, they don't need to remember to go into `services.yml` and add it. The original directories we added to the SE were a random "guess"... which should feel weird. I think it's because that approach is flawed in general.* B) **Less WTF and better DX with `autoconfigure`**: now, if I create a new class that extends `Twig_Extension` but forget to import my `Twig` directory, then my extension won't work. After this, we'll find it for sure.* C) **Clearer for the documentation**: before this change, in many places, we need to say "go to services.yml and add this new directory to your import... unless you already have it... and don't make your line look exactly like our's - you're probably also importing other directories"* D) **We could deprecating the "autowire auto-registration" code in 3.4** (i.e. when autowiring automatically registers a private services if no instance exists in the container). That means less magic: this is actually *more* explicit. This could be awesome - we could remove some "hacky" code (e.g.symfony/symfony#22306). And, if you type-hinted an `Entity` (for example) in autowiring, instead of the container creating a service for you, it can give you a clear errorIn theory, the CON is that this will slow down the container rebuilding as more services are added to `ContainerBuilder` (before being removed later). On one project, I compared the build time without importing everything (138 services added from `src/`) versus importing everything (200 services dded from `src/`). The build time difference was negligible - maybe 10ms difference (both were around 950ms).Btw, the `exclude` key added insymfony/symfony#22680 is almost not even needed, since unused services are simply removed. But, currently, the `Entity` directory is an exception due to the new "argument resolver" trying to see if it can wire those as services. That could be fixed in the future. But `exclude` is kind of nice, if you want to explicitly safe-guard someone from accidentally type-hinting something from that directory. Again, that's *better* than now... where we will *always* auto-register an Entity if you type-hint it.Cheers!Commits-------47d3561 Making *all* services in src/AppBundle available as services
@weaverryanweaverryan deleted the allow-psr4-exclude branchMay 14, 2017 17:58
@fabpotfabpot mentioned this pull requestMay 17, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+2 more reviewers

@mvrhovmvrhovmvrhov left review comments

@GuilhemNGuilhemNGuilhemN approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

9 participants

@weaverryan@nicolas-grekas@stof@javiereguiluz@jvasseur@GuilhemN@fabpot@aurimasniekis@mvrhov

[8]ページ先頭

©2009-2025 Movatter.jp