Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It is *almost* supported. It needs a () and nothing else. So I added that and a test.
Comment | File | Size | Author |
---|---|---|---|
#30 | 1837118_23_0_1.patch | 3.5 KB | chx |
#15 | 1837118_15.patch | 2.82 KB | chx |
#15 | interdiff.txt | 1.94 KB | chx |
#14 | 1837118_14.patch | 2.45 KB | chx |
#14 | interdiff.txt | 1.08 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedMaybe this results in prettier queries.
Comment #2
Crell CreditAttribution: Crell commentedHow I ended up following this without ever seeing it I do not know. Does d.o auto-follow now when an issue is assigned to someone?
Either way, looks sane.
Comment #3
Berdir@Crell: Yes.
Comment #4
chx CreditAttribution: chx commentedComment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe need to prepare this sub-query like we prepare all the others, and make sure that the arguments bubble up properly. It works in the test because the sub-query doesn't use any placeholders.
Comment #6
BerdirRight.
I hope @chx wasn't already working on this, but here's an update that seems to work. Not 100% sure if this is implemented correctly. For some reason that I don't quite understand are we calling compile() on conditions both in execute() and in __toString(). Confirmed with expressions that compile() needs to be called before arguments() but I don't think it needs to be called twice.
I think converting all these database tests, or those that don't require hooks to unit tests or at least DrupalUnitTests would be a nice task, should speed up our tests quite a bit, there's a huge amount of test methods in there...
Comment #7
Crell CreditAttribution: Crell commentedNow that I look more carefully (bad Crell!), why the array_unique() here? That changes what's coming back in ways we cannot detect, so you may get back more than one row and not realize it. If we have to do this it should be commented, because otherwise it seems wrong to me.
I am planning to convert the DB tests to UnitTests post-feature-freeze when I have time. Actually I'll only be converting one of them and setting up the rest as Novice tasks. :-) I've not done that yet because there's other issues messing with the DB, and that's totally not a feature-freeze-sensitive issue so I'm prioritizing my time.
Comment #8
neclimdulWell, it looks sane, the db maintainers think it looks sane and their concerns have been addressed with tests. Lets do it.
Comment #9
chx CreditAttribution: chx commentedAlthough #7 is irrelevant -- it's not the number of rows, it's the contents of them -- I have rerolled the test to look the same as every other test method here.
Comment #10
Berdir@Crell: Had to do something where I was actually able to make progress: #1839134: Convert database tests to (Drupal)UnitTestBase
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedRight. It's because this grew organically. The way it is supposed to work(TM) is:
(string) $query
syntax)Select is about implemented correctly (getArguments() is totally unnecessary and should go away, but that's it).
Also, we need to reimplement
->compiled()
as->compiled($queryPlaceholder)
because we support multiple compilation on the same object now.So, all that to say that we do need the compile in __toString().
Comment #12
BerdirLike this?
Comment #13
alexpottWe need a need for the addition to the __toString() functionality.
Comment #14
chx CreditAttribution: chx commentedSure.
Comment #15
chx CreditAttribution: chx commentedThe test from #9 was lost. I rerolled with that one and added a string test.
Comment #16
alexpott#15 looks great, it's addressed concerns from #11 and #7... and we've got full test coverage for all changed lines of code.
We're good to go here!
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedApproved here.
Comment #18
webchickCommitted and pushed to 8.x. Thanks!
Comment #19
chx CreditAttribution: chx commentedComment #20
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #21
posulliv CreditAttribution: posulliv commentedPatch for D7. Also updated PostgreSQL driver for D7 with appropriate fix.
Comment #22
mgiffordComment #23
chx CreditAttribution: chx commentedReroll.
Comment #24
chx CreditAttribution: chx commentedTry again.
Comment #25
Crell CreditAttribution: Crell commentedNo camelCase for non-object-member variables.
Comment #26
mgiffordComment #27
chx CreditAttribution: chx commentedHow hard it is to download the goddamn patch and edit it? You don't need to do anything else and then we do not hold up a patch that has been linger since 2012! You people.
Comment #28
chx CreditAttribution: chx commentedComment #29
mgiffordThis should be good then. Thanks for rerolling it @chx.
Comment #30
chx CreditAttribution: chx commentedpostgresql had a typo.
Comment #33
chx CreditAttribution: chx commentedComment #36
dcam CreditAttribution: dcam commentedComment #39
dcam CreditAttribution: dcam commentedComment #42
mgiffordComment #45
dcam CreditAttribution: dcam commentedComment #48
dcam CreditAttribution: dcam commentedComment #49
David_Rothstein CreditAttribution: David_Rothstein commentedThis can't possibly work since the variable doesn't exist - perhaps it's the typo @chx intended to fix in #30?
More to the point, these PostgreSQL fixes aren't in Drupal 8... Let's get the issue fixed there first (and tested to make sure it actually works with the relevant database drivers), then backported.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedUNTESTED, but presumably this is what it should look like for Drupal 8. I fixed the issue above (as well as a minor code style issue).
The code looks like it could also be simplified a fair amount (get rid of $select_query_arguments entirely; just use $argument) but this is consistent with the way the code above it is written, so I left that alone.
Comment #51
Crell CreditAttribution: Crell commentedRefiling for the people who know Postgres, if any still pay attention to core...
Comment #52
mradcliffePostgreSQL test bot found this in the failing test: ComplexUpdateTest::testSubSelectUpdate().
Comment #53
bzrudi71 CreditAttribution: bzrudi71 commentedJust ran Database tests on PG testbot. Unfortunately there is still an exception, in Drupal\system\Tests\Database\UpdateComplexTest even with patch from #35 applied, see attached results.txt for a full test log.
Comment #54
mradcliffeSelectQueryInterface is not in drupal 8. Should be SelectInterface. I needed to include the interface for the test to pass.
Comment #55
mradcliffeForgot to rebase like a scrub. Sorry, Crell.
Comment #56
Crell CreditAttribution: Crell commentedIf I understand #53 correctly PG is already broken and this doesn't make it worse. However, the latest patch is missing the tests from earlier issues.
Comment #57
Fabianx CreditAttribution: Fabianx commentedI don't totally understand how binding works, so RTBC if tests pass and it has all tests to test that ...
Comment #58
mradcliffeClosing as fixed in Drupal 8 per Crell.
I will create a follow-up issue specifically for PostgreSQL Drupal 8 and Drupal 7. It also looks like this patch needs to be backported to Drupal 7 in general so I will create a follow-up issue on that.
- Hiding files that were added after issue was closed to be less confusing.
Comment #59
Fabianx CreditAttribution: Fabianx commentedWhat is the process for PG SQL then?
It was not yet removed from core as much as I can see and we can ship it if:
a) tests pass
and
b) we have PG SQL test bots
So why closing that issue?
Comment #60
mradcliffeI created #2350461: Fix issues with UpdateComplexTest in PostgreSQL driver for postgresql in D8 and #2350465: UPDATE foo SET bar=(SELECT...) is not supported in D7? per request by Crell.
Comment #61
Fabianx CreditAttribution: Fabianx commentedOkay, thanks. This is a duplicate then.
Comment #62
Crell CreditAttribution: Crell commentedComment #63
Fabianx CreditAttribution: Fabianx commentedSorry for playing ping-pong, but would like to know why: Closed (fixed) if there are other issues that are duplicates of this one?
Comment #64
Fabianx CreditAttribution: Fabianx commentedAh, something was fixed in this issue already - and committed. Makes more sense now.
Leaving as closed fixed then.