-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Oracle add duplicate slashes for ClassType of ObjectIdentity in ACL provider #11
Comments
Hi @derrabus, yes around but not using Oracle anymore the fix looks alright though :) |
Hi @derrabus, will you accept the PR? :) |
It looks good, but I don't have a setup to verify it. 🤔 |
@derrabus The rest is standard. |
@derrabus |
My main issue is that we don't have a maintainer with Oracle knowledge or a CI that runs tests against Oracle on this repo. I don't feel like I'm the right person to merge this. |
Hi @derrabus |
It would be great if we can refactor https://github.com/symfony/security-acl/blob/main/Tests/Dbal/AclProviderTest.php to test on Oracle, MySQL and sqlite (current) on GitHub actions (and skip the particular database if any of them is not supported locally). That would give us much more confidence that the fix works and we're not breaking it in the future. |
I tried to do so on the ORM, since we already do have Oracle test on the DBAL. My attempt can be seen at doctrine/orm#8958 |
Hmm, that's a bit of a bummer... In that case, we rely on the community. Currently, #96 is open without any reviewers. If anyone that has Oracle locally can verify this indeed fixes this annoying bug, I would be much more confident when merging the PR. |
Not using Oracle anymore but the bug in 2015 was there and the fix I proposed was the same as the one in #96 I'd be 👍 to merge it. |
symfony/symfony#15723 follow up
Indeed, when
$this->connection->quote
is used on theclassType
, Oracle adds two back-slashes.I've found a simple fix that is to
stripcslashes($classType)
on hydrateObjectIdentities. But this should only apply when the$classType
contains more than one\
.Thoughts?
The text was updated successfully, but these errors were encountered: