Closed (fixed)
Project:
Drupal core
Version:
9.1.x-dev
Component:
entity system
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
29 Dec 2020 at 20:59 UTC
Updated:
7 Feb 2021 at 07:14 UTC
Jump to comment: Most recent, Most recent file
Show commands
changes
Comments
Comment #2
larowlanShould this be a private issue?
Comment #3
larowlanUnpublished just in case
Comment #4
mondrake@larowlan no I don't think so, this is a database portability issue, not a security related one
Comment #5
mondrakeTest only patch.
Comment #6
mondrakeComment #7
larowlanComment #8
mondrakeComment #10
mondrakeComment #11
mondrakeSo, the test in #5 has nothing to do with the issue here, as it releates to queries generated by views and not by the entity system.
The latest MR, though, allows
EntityQueryAggregateTestto pass successfully on Oracle.Comment #12
mondrakeComment #13
mondrakeTests could be improved if #3188918: Allow EntityQuery to be converted to the underlying SQL query was is.
Comment #14
mondrakeComment #15
daffie commented@mondrake: The pacth looks good, just one remark.
Comment #17
anmolgoyal74 commentedComment #18
daffie commentedThe patch looks great to me.
For me it is RTBC.
Comment #19
mondrakeI have a failure in testing my DruDbal contrib driver when using the latest MR, due to mismatching quote identifiers. IMHO we should go back to just check that 'an' identifier is present, not hardcoding the SELECT.
Comment #20
daffie commented@mondrake: Could you change the test, so that it also passes for your DruDbal driver?
Comment #21
mondrake@daffie it was passing before the last change
Comment #22
daffie commentedI would very much like to test on the identifierQuotes from the connection class. Testing on
'\(.entity_test.\..id.\) AS .id_'is not ver clear to me. For me the problem with the regexp assertion is that regexp sees double quotes as special characters. Therefore an assertStringContainsString() would be better.What are the values of $start_quote and $end_quote for your DruDbal driver? If they are empty/not set could we make double quotes their default values? Would that work for your driver?
If we remove the 'SELECT' part from my variable $expected_query_part, would that work for your DruDbal driver?
Comment #23
mondrakeIn DruDbal the identifierQuotes are not fixed - they're dependent on the actual DBMS doctrine/dbal connects to - from backtick in MySql to double quote in SQLite and Oracle.
I do not think we should test the identifierQuotes in this test - if need be, I'd rather go for a separate, independent test of its own.
The dot in the regex is there to mean 'is there any character in this position', so for me it's OK since we need to ensure that e.g.
max([entity_test].[id]) AS [id_max]is converted to
max("entity_test"."id") AS "id_max"ormax(`entity_test`.`id`) AS `id_max`ormax('entity_test'.'id') AS 'id_max'i.e. that at least a character exist before and after the table, column and alias identifiers - which is not the case currently.
Comment #24
daffie commented@mondrake: OK, lets go with your solution. Could you add a comment with your explanation from comment #23 to the regexp test in the patch.
Comment #25
mondrakeon it
Comment #26
mondrakeDone, thanks!
Comment #27
mondrakeTwo separate random test failures in a row...
Comment #28
daffie commentedIt all looks good to me.
For me it is RTBC.
Comment #29
mondrakeFixed @catch's nit. Please change the status of the issue or at least add a comment to the issue when adding comment to the PR, ATM these are not refreshing the timestamp and are not visible from the issue queue - you really need to get into the issue and MR to notice them.
Comment #32
catchThanks for fixing the nit.
Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!