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

Don't call deprecated getSQLResultCasing and usesSequenceEmulatedIdentityColumns when we know the platform #10759

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

nicolas-grekas
Copy link
Member

Saves triggering the deprecation on PHP 7 where only DBAL v2 can be used:

"AbstractPlatform::getSQLResultCasing is deprecated without replacement and removed in DBAL 3.Use Portability\Connection with PORTABILITY_FIX_CASE to get portable result cases. (AbstractPlatform.php:3515 called by SQLResultCasing.php:30, doctrine/dbal#4229, package doctrine/dbal)": 246,

@nicolas-grekas nicolas-grekas force-pushed the deprec-case branch 6 times, most recently from 564c434 to c90f161 Compare June 5, 2023 12:33
@greg0ire
Copy link
Member

greg0ire commented Jun 5, 2023

Hi! Before you spend too much effort on this: I think this comment applies here too: no one should be using PHP 7 anymore, and the same holds true for DBAL 2. That being said, there are already a bunch of if ($platform instanceof…) in here, so it's maybe not that straightforward. @derrabus , what your thoughts on this?

@nicolas-grekas
Copy link
Member Author

That's not the same. This covers the situation where DBAL v2 is used with ORM v2.15. Right now, ORM calls a deprecated API of DBALv2 while it can and thus should avoid doing so when possible, which is the case covered here.

@greg0ire
Copy link
Member

greg0ire commented Jun 5, 2023

Ok well, you managed to get a green build anyway 👍
@derrabus authored that piece of code, so I'll defer to him.

@derrabus
Copy link
Member

derrabus commented Jun 5, 2023

The rationale behind keeping the call was that the platform classes are in fact extension points in DBAL. You can override SQLServerPlatform if you want to change how the DBAL interacts with SQL Server for instance. A consumer of DBAL could override getSQLResultCasing() in its own platform.

Your instanceof checks would not detect that case and break such an app, I'm afraid.

@nicolas-grekas
Copy link
Member Author

That's why I checked explicitly the platforms we know about: changing a child class won't change the behavior of the DB engine, so that there is nothing to allow customizing in this regard for the listed platforms.

@nicolas-grekas
Copy link
Member Author

PR updated to also skip calling deprecated usesSequenceEmulatedIdentityColumns in ClassMetadataFactory

@derrabus
Copy link
Member

derrabus commented Jun 5, 2023

there is nothing to allow customizing in this regard for the listed platforms.

I agree and that's probably one of the reasons this method has been removed. I wonder if we can instead find a better way to silence that deprecation instead.

@nicolas-grekas
Copy link
Member Author

Why would we need another way? We know the server's behavior => done.

@nicolas-grekas
Copy link
Member Author

(just rebased)

@nicolas-grekas nicolas-grekas changed the title Don't call deprecated getSQLResultCasing when we know the platform Don't call deprecated getSQLResultCasing and usesSequenceEmulatedIdentityColumns when we know the platform Jun 6, 2023
@nicolas-grekas nicolas-grekas force-pushed the deprec-case branch 3 times, most recently from 210c43a to 56db246 Compare June 6, 2023 09:14
@derrabus derrabus added the Bug label Jun 6, 2023
@derrabus derrabus added this to the 2.15.3 milestone Jun 6, 2023
@derrabus derrabus merged commit 3827dd7 into doctrine:2.15.x Jun 6, 2023
@nicolas-grekas nicolas-grekas deleted the deprec-case branch July 7, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants