-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 public api for Doctrine QueryBuilder and ExpressionBuilder #17413
Conversation
Looks good so far :) |
Great first shot - any chance to get quoting handled within the wrapper as well? |
I would say this PR is big enough. We should not add more features to this one. |
Mhm. If it is a new public API I'd say we should first decide if we want quoting or not. If we do then we should do it right at the first time before others rely on the behaviour. |
yes |
Ah okay ... got it :( 🙈 I completely misunderstood that thing ... |
I shall have a look after finishing the walk through my emails and other open tasks |
$connection = \OC::$server->getDatabaseConnection(); | ||
$query = $connection->createQueryBuilder(); | ||
$query = $connection->getQueryBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one is the Doctrine thing? create or get? Just for an overview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create is doctrine, get is OC
Okay, there are some problems which I found, while having a quick look/thought: E.g.
Can be fixed by forcing the use of However, that method itself has the problem as well:
Theory case okay, back to practise: $expr->eq('column', 'strangestring') Should we escape the second string? This can be prevented by forcing the use of ConclusionSounds doable, I shall go ahead and do that. |
f521dd9
to
ee9a026
Compare
Tests failed due to a bug in Doctrine/DBAL which is already fixed since January but has not yet been released: doctrine/dbal#782 |
affc1d4
to
ca91ed4
Compare
Jenkins passed here #17739 (comment) - https://ci.owncloud.org/job/pull-request-analyser-ng-simple/13656/ |
Quoting is now included Ready for review and merge @DeepDiver1975 @LukasReschke @MorrisJobke |
Open question from IRC: Should we break the existing I don't think it's worth the troubles it may cause. |
Correct. It was used by some apps ... better keep it compatible now but maybe have a future version of Alternative: Maybe also worth to add to the app code checker. ;) |
Okay, sounds like a plan. Will make a wrapper and add a debug log entry.
Sadly enough, this is exactly the one case that is not covered ;) |
9352eec
to
20cd0ae
Compare
A new inspection was created. |
Very awesome! Also awesome that the existing code is converted (I wrote some of that). |
Looks good and a smoke test also just went fine 👍 |
Add public api for Doctrine QueryBuilder and ExpressionBuilder
Replaces #17383
Related to #12502
cc @LukasReschke @DeepDiver1975 @schiesbn @Raydiation