Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moves default and user-configured overridden bundle path registration to a CompilerPass #30360

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
6 participants
@alterphp
Copy link

alterphp commented Feb 23, 2019

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30359
License MIT

If #30359 is indeed considered as bug or problem, this PR is a solution to solve it.

TwigBundle default overriden bundle paths are defined too early in the bundle definition process. They are now configured in a CompilerPass that allows 3rd party bundles to override any Twig namespace and let the user have its own project overrides.

According to @yceruto request, I also moved the user-configured paths to the compiler pass.

@alterphp alterphp force-pushed the alterphp:fix/30359 branch from a115b64 to 81e291a Feb 23, 2019

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Feb 23, 2019

@OskarStark
Copy link
Contributor

OskarStark left a comment

Are you sure this is only for 4.2 and no bugfix for 3.4 branch which is also maintained?

Show resolved Hide resolved ...igBundle/DependencyInjection/Compiler/DefaultOverridenBundlePathPass.php Outdated
}
}
private function normalizeBundleName($name)

This comment has been minimized.

Copy link
@OskarStark

OskarStark Feb 23, 2019

Contributor
Suggested change
private function normalizeBundleName($name)
private function normalizeBundleName(string $name): string

but substr() could also return false... what do do here then @nicolas-grekas ?

This comment has been minimized.

Copy link
@alterphp

alterphp Feb 23, 2019

Author

@OskarStark @nicolas-grekas I just copied the method from TwigExtension. Then I don't like copying the logic... Any suggestion on this ?

Show resolved Hide resolved ...igBundle/DependencyInjection/Compiler/DefaultOverridenBundlePathPass.php Outdated
@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Feb 23, 2019

And btw there are two typos in the PR title:

- Moves default overriden bnudle path registration to a CompilerPass
+ Moves default overridden bundle path registration to a CompilerPass
@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Feb 23, 2019

Are you sure this is only for 4.2 and no bugfix for 3.4 branch which is also maintained?

@OskarStark At least 4.x and master. For 3.4, I have to check. Should i list target branches in the head Q/A section ?

@alterphp alterphp changed the title Moves default overriden bnudle path registration to a CompilerPass Moves default overridden bundle path registration to a CompilerPass Feb 23, 2019

alterphp added some commits Feb 23, 2019

@alterphp alterphp changed the title Moves default overridden bundle path registration to a CompilerPass Moves default and user-configured overridden bundle path registration to a CompilerPass Feb 25, 2019

alterphp added some commits Feb 25, 2019

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 5, 2019

@yceruto @OskarStark Any update on this one ?

$bundleHierarchy[$name][] = $defaultOverrideBundlePath;
}
$container->addResource(new FileExistenceResource($defaultOverrideBundlePath));
if (file_exists($dir = $bundle['path'].'/Resources/views')) {

This comment has been minimized.

Copy link
@yceruto

yceruto Mar 5, 2019

Contributor

Hi @alterphp, re-thinking this in another way... instead, what about moving this registration (as late as possible) into the compiler pass and call to addPath always?

I mean, G1 and G2 as before in the same place, and only G3 into the compiler pass, thus other bundles would addPath before G3 keeping the priority/order and also less changes than now.

This comment has been minimized.

Copy link
@alterphp

alterphp Mar 5, 2019

Author

It took me some minutes to understand ... and it sounds far better than what I have done until now. I'm gonna take the time to refactor this.

Thanks for review !

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 5, 2019

I also think this is more a new feature (master branch) rather than a bugfix, because you're improving the paths registration process to allow bundles "easily" insert new "override" paths for other bundles and actually you've altenatives #30360 (comment)

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Mar 11, 2019

Closed in favor or #30527

@alterphp alterphp closed this Mar 11, 2019

@alterphp alterphp deleted the alterphp:fix/30359 branch Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.