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

Update priority of built-in middleware #1760

Merged
merged 1 commit into from Feb 21, 2024

Conversation

l-vo
Copy link
Contributor

@l-vo l-vo commented Feb 18, 2024

With the default priority, middlewares for logging and profiler don't see the eventual query changes performed by userland middlewares (that have by default a priority of 0)

@ostrolucky ostrolucky added this to the 2.12.0 milestone Feb 20, 2024
@ostrolucky ostrolucky changed the base branch from 2.11.x to 2.12.x February 20, 2024 20:37
@ostrolucky
Copy link
Member

Nature of this is dubious, plus potentially BC breaking. Targetting 2.11.x. Not sure it should have prio 100 either, that's quite high. Something like 10 would be better I reckon.

@l-vo
Copy link
Contributor Author

l-vo commented Feb 20, 2024

@ostrolucky yes it can be 10, I was not sure about the number.

Why do you think it's dubious ?

@ostrolucky
Copy link
Member

Because doctrine-bundle never claimed to log everything, including custom middlewares. For some people it might be intended that queries in custom middlewares are not logged.

@l-vo
Copy link
Contributor Author

l-vo commented Feb 20, 2024

@ostrolucky it makes sense 👍

So I close it, thank you for your feedback :)

@l-vo l-vo closed this Feb 20, 2024
@l-vo l-vo deleted the update_builtin_midlewares_priority branch February 20, 2024 21:21
@ostrolucky
Copy link
Member

Well I agree with the change, my only argument was to not classify this as a bugfix, which I did ;)

@l-vo
Copy link
Contributor Author

l-vo commented Feb 20, 2024

But there is also the BC break problem, isn't it ?

@ostrolucky
Copy link
Member

It's a potential break for someone. But middleware priorities is not something we promise not to change.

@l-vo l-vo restored the update_builtin_midlewares_priority branch February 21, 2024 08:11
@l-vo
Copy link
Contributor Author

l-vo commented Feb 21, 2024

@ostrolucky ok I see 👍 I reopened it with the priority set to 10. Thank you for your feedbacks :)

@l-vo l-vo reopened this Feb 21, 2024
@l-vo l-vo force-pushed the update_builtin_midlewares_priority branch from eebaeec to 58e0119 Compare February 21, 2024 08:18
With the default priority, middlewares for logging and profiler don't see the eventual query changes performed by userland middlewares (that have by default a priority of 0)
@l-vo l-vo force-pushed the update_builtin_midlewares_priority branch from 58e0119 to 0128804 Compare February 21, 2024 08:40
@ostrolucky ostrolucky merged commit c081482 into doctrine:2.12.x Feb 21, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants