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

Add CompilerPass with priority to TestKernel #77

Closed
FelixJacobi opened this issue Apr 26, 2023 · 3 comments · Fixed by #88
Closed

Add CompilerPass with priority to TestKernel #77

FelixJacobi opened this issue Apr 26, 2023 · 3 comments · Fixed by #88

Comments

@FelixJacobi
Copy link

Currently, you cannot set the $priority argument of ContainerBuilder::addCompilerPass(); when adding a CompilerPass to the TestKernel. In a few cases, this is needed, e.g. to run after another CompilerPass which alters the container (in my case: I am developing a bundle providing a custom translator implementation, before we can alter the translator in the pass of the bundle, the CompilerPass of the FrameworkBundle, which configures the Translator, must have ran).

I see these possibilities here:

  • Add a new method with more arguments (no BC break, but probably bad wording)
  • Change existing method (BC break, but still clear wording)
  • Or something else...?

I am willing to work on this, but I am not sure, which way shall I choose.

@chapterjason
Copy link
Contributor

@chapterjason chapterjason mentioned this issue Dec 9, 2023
9 tasks
@chapterjason
Copy link
Contributor

Hey, while working on the implementation, I realized that we have our own layer for adding compiler passes to the kernel. It's actually simpler than I initially thought. Take a look at #88

@chapterjason chapterjason linked a pull request Dec 16, 2023 that will close this issue
1 task
@FelixJacobi
Copy link
Author

FelixJacobi commented Dec 20, 2023

Hi @chapterjason,

Thank you for working on this. Sadly, I don't exactly know, where I needed this feature, since I worked with a lot of bunches of code during that time. But AFAIR: It looks familiar to my workaround from the past, so that my original problem should be solved with that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants