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

fix: sort query parameters by default #6328

Closed
wants to merge 1 commit into from

Conversation

drupol
Copy link

@drupol drupol commented Mar 6, 2024

Q A
Type bug
Fixed issues Related to symfony/symfony#54172

Summary

Fixing the file //OCI8//ConvertPositionalToNamedParameters.php was enough for our use case, and I confirm that our query is correctly executed now. I also confirm that the initial PR is now optional (symfony/symfony#54172), however I think it's a nice to have. Dealing with named parameters is safer and easier to reason about when reading the code.
I took the liberty to also sort other methods, let me know if I did well or not.

Last question, what's the policy to backport this fix in older versions?

@drupol drupol marked this pull request as ready for review March 6, 2024 14:38
@derrabus
Copy link
Member

derrabus commented Mar 6, 2024

Thank you. Please provide a functional test that reproduces the issue you're trying to fix.

@derrabus
Copy link
Member

derrabus commented Mar 6, 2024

Last question, what's the policy to backport this fix in older versions?

We fix bugs on the oldest supported version that is affected by the bug and then merge up from there. That means, if your bug is reproducible on 3.8.x, please retarget your PR.

@drupol

This comment was marked as resolved.

@derrabus
Copy link
Member

derrabus commented Mar 6, 2024

It's reproducible from version 2 😞

Which is why I said, "the oldest supported version". That is 3.8 at the moment.

@drupol drupol force-pushed the fix/oci8/sort-parameters branch 3 times, most recently from 7ef68e5 to d7fa81f Compare March 7, 2024 07:34
@drupol drupol marked this pull request as draft March 7, 2024 10:39
@drupol
Copy link
Author

drupol commented Mar 7, 2024

Setting the PR to draft, currently unable to make a reproducer in the tests.

@drupol
Copy link
Author

drupol commented Mar 7, 2024

Hey all,

I wanted to provide an update.

After further investigation and numerous attempts to reproduce the issue, including starting from a blank application state and trying to replicate the conditions under which the issue was initially observed, my colleague and I have come to a conclusion that we are now unable to reproduce the issue in any form. Perhaps, this indicates that the problem may have been influenced by factors or states within our specific project setup that we could not accurately replicate in isolated testing, but as of today, we are totally unable to reproduce it.

Given this, this PR is now obsolete, and I'm closing it.

Thanks.

@drupol drupol closed this Mar 7, 2024
@drupol drupol deleted the fix/oci8/sort-parameters branch March 7, 2024 14:39
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