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 the hook_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() with db_merge().
  • Remove drupal_write_record().

Related issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

If we remove drupal_write_record() we also need to remove hook_schema_alter().

sun’s picture

Issue tags: +Novice

I actually think the primary conversions are pretty easy and thus novice material.

_12345678912345678’s picture

I will apologize in advance since I am still new to this site but where can I download this to work on it.

webchick’s picture

Issue tags: -Novice

I'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! :)

rbayliss’s picture

Is there any easy way to get the last insert ID's from db_merge()? Otherwise, we're going to be looking at converting this:

if (empty($path['pid'])) {
  drupal_write_record('url_alias', $path);
  module_invoke_all('path_insert', $path);
}
else {
  drupal_write_record('url_alias', $path, array('pid'));
  module_invoke_all('path_update', $path);
}

with this:

$status = db_merge('url_alias')
    ->key(array('pid' => $path['pid']))
    ->fields(array(
      'source' => $path['source'],
      'alias' => $path['source'],
      'langcode' => $path['langcode'],
    ))
    ->execute();
  if($status == Merge::STATUS_INSERT) {
    $path['pid'] = db_query("SELECT pid FROM {url_alias} WHERE alias = :alias", array(':alias' => $path['alias']));
    module_invoke_all('path_insert', $path);
  }
  else {
    module_invoke_all('path_update', $path);
  }

This seems really nasty.

agentrickard’s picture

Could 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.

sun’s picture

Component: base system » database system
Assigned: Unassigned » Crell
FileSize
2.13 KB

Hm. So we'd ideally want something like this:

$query = db_merge('url_alias')
  ->key(array('pid' => $path['pid']))
  ->fields(array(
    'source' => $path['source'],
    'alias' => $path['source'],
    'langcode' => $path['langcode'],
  ));
$status = $query->execute();
if($status == Merge::STATUS_INSERT) {
  $path['pid'] = $query->getID();
  module_invoke_all('path_insert', $path);
}
else {
  module_invoke_all('path_update', $path);
}

Attached patch shows a prototype, but I suspect there is no RETURN_INSERT_ID for UPDATE queries.

Crell’s picture

+++ b/core/lib/Drupal/Core/Database/Query/Merge.php
@@ -414,13 +419,14 @@ public function execute() {
-          $insert = $this->connection->insert($this->table)->fields($this->insertFields);
+          $insert = $this->connection->insert($this->table, $options)->fields($this->insertFields);

I believe insert queries force this already themselves, don't they?

+++ b/core/lib/Drupal/Core/Database/Query/Merge.php
@@ -442,7 +448,7 @@ public function execute() {
-        $update->execute();
+        $this->returnID = $update->execute();

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.

sun’s picture

I'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:

if (empty($path['pid'])) {
  drupal_write_record('url_alias', $path);
  module_invoke_all('path_insert', $path);
}
else {
  drupal_write_record('url_alias', $path, array('pid'));
  module_invoke_all('path_update', $path);
}

simply needs to become this:

$query = db_merge('url_alias')
  ->key(array(
    'pid' => isset($path['pid']) ? $path['pid'] : NULL,
  ))
  ->fields(array(
    'source' => $path['source'],
    'alias' => $path['source'],
    'langcode' => $path['langcode'],
  ));
if ($query->execute() == Merge::STATUS_INSERT) {
  $path['pid'] = $query->getInsertID();
  module_invoke_all('path_insert', $path);
}
else {
  module_invoke_all('path_update', $path);
}

I've to agree it looks considerably more horrible. ;)

sun’s picture

Crell’s picture

Assigned: Crell » Unassigned
_12345678912345678’s picture

Thanks. 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.

_12345678912345678’s picture

Boy 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.

_12345678912345678’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

sun’s picture

Title: Replace all drupal_write_record() calls with db_merge() and get rid of drupal_write_record() » Replace all trivial drupal_write_record() calls with db_merge()
Assigned: Unassigned » sun
Status: Active » Needs review
FileSize
11.35 KB

→ Replaced all trivial instances of drupal_write_record() with proper Database Query builders.

The remaining, more complex scenarios are not touched by this patch:

Entity\DatabaseStorageController::save()
Entity\FieldableDatabaseStorageController::save()
Entity\FieldableDatabaseStorageController::saveRevision()
menu_link\MenuLinkStorageController::save()

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()

sun’s picture

FileSize
11.34 KB
1.51 KB

Oops. drupal_get_schema() force-removes all 'description' keys from schema information.

The last submitted patch, 15: dwr.15.patch, failed testing.

sun’s picture

Work 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()

Status: Needs review » Needs work

The last submitted patch, 16: dwr.16.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.51 KB
1014 bytes

Use explicit fields in locale_translation_update_file_history(), since $file is a random dumping ground.

Status: Needs review » Needs work

The last submitted patch, 20: dwr.20.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.45 KB
533 bytes

Fixed $file in locale_translation_update_file_history() doesn't have a $filename and $uri.

sun’s picture

Green! :-)

The minor DX improvement to Merge::fields() is not strictly required anymore for this patch, since I had to replace the only instance of db_merge() to use an explicit fields() list in the end... The minor change allows you to do this:

db_merge('mytable')
  ->key(array('id' => $thing->id))
  ->fields((array) $thing)
  ->execute();

The fields in fields() that are already contained in key() are ignored for the UPDATE query, so you do not have to manually remove the key fields from $thing prior to calling fields().

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.

sun’s picture

FileSize
10.35 KB
1.1 KB

Actually, let me remove that unnecessary change to the Merge query class, so as to prevent this fine patch from being hold up on that. ;)

dawehner’s picture

+++ b/core/modules/locale/locale.module
@@ -924,22 +924,23 @@ function locale_translation_get_file_history() {
+  $status = db_merge('locale_file')
+    ->key(array(
+      'project' => $file->project,
+      'langcode' => $file->langcode,
+    ))
+    ->fields(array(
+      'version' => $file->version,
+      'timestamp' => $file->timestamp,
+      'last_checked' => $file->last_checked,
+    ))
+    ->execute();

Any reason we don't care about the filename?

sun’s picture

The $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).

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

mh okay.

The last submitted patch, 7: drupal8.db-merge-id.7.patch, failed testing.

andypost’s picture

locale generates filename so not care, it's not used here

jibran’s picture

24: dwr.24.patch queued for re-testing.

Crell’s picture

In 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.

sun’s picture

Yeah, 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

catch’s picture

Status: Reviewed & tested by the community » Needs review

If 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.

tim.plunkett’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community

Yay, thanks for that pointer :) Since that was the only remark, I guess that means we're back to RTBC?

ianthomas_uk’s picture

Status: Reviewed & tested by the community » Postponed

If 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.

sun’s picture

Status: Postponed » Reviewed & tested by the community

I'm not sure whether you looked at the patch, because you're missing something big:

  1. The queries being touched by this patch only belong to tests and API/hook documentation.
  2. There is only one exception in functional code of Locale module, but Locale module uses direct database queries elsewhere in its code already.
  3. This patch does not touch the more complex usages of drupal_write_record() in e.g. the entity field controllers.
webchick’s picture

Assigned: sun » catch

Let's see what catch says to that.

sun’s picture

To clarify:

drupal_write_record() + hook_schema_alter() only makes sense if all corresponding database queries are fully based on the hook_schema() information, too (which is accomplished via drupal_schema_fields_sql()).

That is not the case here. And of course, the tests + API docs are irrelevant.

ianthomas_uk’s picture

RE #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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.