Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem
drupal_write_record()
is a pretty lame data-schema binding/conversion attempt, which which doesn't take the actual entity properties into account and performs a "dumb" transformation of object properties into schema column values.drupal_write_record()
depends on thehook_schema()
definition for the storage table, which is hardly needed otherwise and a CPU/memory pain to (re-)compute and cache.
Goal
- Replace all remaining calls to
drupal_write_record()
withdb_merge()
. - Remove
drupal_write_record()
.
Related issues
Comment | File | Size | Author |
---|---|---|---|
#24 | interdiff.txt | 1.1 KB | sun |
#24 | dwr.24.patch | 10.35 KB | sun |
#22 | interdiff.txt | 533 bytes | sun |
#22 | dwr.22.patch | 11.45 KB | sun |
#20 | interdiff.txt | 1014 bytes | sun |
Comments
Comment #1
catchIf we remove drupal_write_record() we also need to remove hook_schema_alter().
Comment #2
sunI actually think the primary conversions are pretty easy and thus novice material.
Comment #3
_12345678912345678 CreditAttribution: _12345678912345678 commentedI will apologize in advance since I am still new to this site but where can I download this to work on it.
Comment #4
webchickI'd like to remove this from the Novice queue until/unless there's buy-in that this is a good idea. hook_schema_alter() is very handy. We should pull in the Migrate module folks, as AFAIK they rely on this function extensively.
@greenroomwebdesign: There's some good information at http://drupal.org/patch about contributing patches to Drupal.org, or feel free to stop by Core Contribution Mentoring hours on IRC for more hands-on help! :)
Comment #5
rbayliss CreditAttribution: rbayliss commentedIs there any easy way to get the last insert ID's from db_merge()? Otherwise, we're going to be looking at converting this:
with this:
This seems really nasty.
Comment #6
agentrickardCould we instead replace the ugly guts of drupal_write_record() with a db_merge() statement and some handler code, to allow for the return functionality noted in #5? Removing that simple way to get an insert id is a nasty DX regression.
Comment #7
sunHm. So we'd ideally want something like this:
Attached patch shows a prototype, but I suspect there is no RETURN_INSERT_ID for UPDATE queries.
Comment #8
Crell CreditAttribution: Crell commentedI believe insert queries force this already themselves, don't they?
Update returns the number of rows affected, not the ID of the row affected, so in this case the return value is not useful in getId.
Insert and Update return very different things. Update returns affected rows. Insert returns the generated auto-increment ID, or undefined if there was no auto-increment ID. However, merge queries on an auto-increment table don't make a great deal of sense. In practice, I don't think it's possible to take that approach.
That said, I'm very +1 on exterminating drupal_write_record() and hook_schema_alter(). Step one should be to just replace the easy places with merge queries, and see what's left.
Comment #9
sunI'm not sure whether that resolves the practical DX issues of db_merge() outlined in the code snippets in earlier comments.
The serial column is rather irrelevant; what matters is drupal_write_record()'s current facility of automatically (re-)assigning the column values to the passed in variable.
Of course, you could argue that this:
simply needs to become this:
I've to agree it looks considerably more horrible. ;)
Comment #10
sunRelated: #1933686: Why are entities still queried based on hook_schema()?
Comment #11
Crell CreditAttribution: Crell commentedComment #12
_12345678912345678 CreditAttribution: _12345678912345678 commentedThanks. I appreciate the info. Sorry I didn't reply sooner was swamped in work for a bit. One more question about mentoring hours. I am in EST Maine so what time in my time is the mentoring hours in irq. I showed up at the time it listed not realizing it was another time zone and was in a empty room.
Comment #13
_12345678912345678 CreditAttribution: _12345678912345678 commentedBoy it really is hard staying up for the newcomers meeting. In this time zone it is like 2 am. I keep trying to stay up for it and keep falling asleep. I wish there was a meeting sometime durning the day in the eastern standard time zone.
Comment #13.0
_12345678912345678 CreditAttribution: _12345678912345678 commentedUpdated issue summary.
Comment #14
sunComment #15
sun→ Replaced all trivial instances of
drupal_write_record()
with proper Database Query builders.The remaining, more complex scenarios are not touched by this patch:
Those calls pretty much fall into the scope of the parallel effort of replacing
hook_schema()
with base field definitions.For the sake of making progress, I'm reducing the scope of this issue to exactly these simple changes, deferring the removal to #2194885: Remove drupal_write_record()
Comment #16
sunOops. drupal_get_schema() force-removes all 'description' keys from schema information.
Comment #18
sunWork on this issue made me remember that I wanted to file the following patch for a very long time already:
#2194897: Rename Database\Query\Merge::key() to keys(), retain a BC alias for key()
Comment #20
sunUse explicit fields in locale_translation_update_file_history(), since $file is a random dumping ground.
Comment #22
sunFixed $file in locale_translation_update_file_history() doesn't have a $filename and $uri.
Comment #23
sunGreen! :-)
The minor DX improvement to
Merge::fields()
is not strictly required anymore for this patch, since I had to replace the only instance ofdb_merge()
to use an explicitfields()
list in the end... The minor change allows you to do this:The fields in
fields()
that are already contained inkey()
are ignored for the UPDATE query, so you do not have to manually remove the key fields from$thing
prior to callingfields()
.I'm happy to remove that change from this patch and move it into a separate issue, if anyone thinks that it can't go in as part of this patch.
Comment #24
sunActually, let me remove that unnecessary change to the
Merge
query class, so as to prevent this fine patch from being hold up on that. ;)Comment #25
dawehnerAny reason we don't care about the filename?
Comment #26
sunThe
$file
that is being passed into this function does not have a filename property/value - see test failures in #16. That's why I had to replace the fields with an explicit list in #20 (see interdiff).Comment #27
dawehnermh okay.
Comment #29
andypostlocale generates filename so not care, it's not used here
Comment #30
jibran24: dwr.24.patch queued for re-testing.
Comment #31
Crell CreditAttribution: Crell commentedIn case it's needed, here's another +1 for this. I've wanted to kill drupal_write_record() since Drupal 6. :-) sun, please do file a new issue for the merge query tweak. I'm not sure yet how I feel about it, so let's put it in its own issue to discuss further.
Comment #32
sunYeah, I'm no longer sure about that proposal myself either, but I've created #2203031: Exclude fields in Merge::fields() that were already passed to Merge::key() for the UPDATE query
Comment #33
catchIf we're going to do this, we need to remove hook_schema_alter(). That's not been discussed anywhere so let's please open an issue for that before it just magically stops working.
Comment #34
tim.plunkett#2060629: Remove hook_schema_alter() from core exists already.
Comment #35
sunYay, thanks for that pointer :) Since that was the only remark, I guess that means we're back to RTBC?
Comment #36
ianthomas_ukIf this is going to break hook_schema_alter(), then I don't see how we can do this issue first.
Postponing on #2060629: Remove hook_schema_alter() from core. Discussion currently in there (#19-#24) may even make this a won't fix.
Comment #37
sunI'm not sure whether you looked at the patch, because you're missing something big:
drupal_write_record()
in e.g. the entity field controllers.Comment #38
webchickLet's see what catch says to that.
Comment #39
sunTo clarify:
drupal_write_record()
+hook_schema_alter()
only makes sense if all corresponding database queries are fully based on thehook_schema()
information, too (which is accomplished viadrupal_schema_fields_sql()
).That is not the case here. And of course, the tests + API docs are irrelevant.
Comment #40
ianthomas_ukRE #37: No, I didn't read the patch, but I did see core maintainers in #1, #4 and #33 expressing concern about hook_schema_alter(), and the only response to #33 (before #39) being a link to an unfixed issue to remove hook_schema_alter().
This seems a reasonable optimisation for tests.
I only think it's a positive change for API documentation (which we are expecting people to copy-paste) and the locale module if we drop hook_schema_alter().
I think catch is already looking at this though, so I'll leave it to him.
Comment #41
catchOK with this patch as it is. Still think the other ones need to demonstrate they aren't jumping the gun.
Committed/pushed to 8.x, thanks!