Problem/Motivation

When a log is automatically given a name using the log type's "Name pattern", it can use tokens based on the log's fields.

If these fields change and the log is re-saved, then the log's name will not reflect the changes.

Steps to reproduce

Imagine a "Harvest" log type with an "Area reference" field, and a "Name pattern" of Harvest from [area:name]. If I create a new harvest log that references "Area A", the log name will be "Harvest from Area A". If I then change the reference to "Area B" and resave the log, the log's name is still "Harvest from Area A"

Proposed resolution

We should try to regenerate names when logs are updated.

At quick glance, this is a bit tricky, because we only generate a log's name if it is empty when it is saved. Furthermore, we don't want to overwrite a name that was manually entered by a user. When our postSave method runs on an existing log, it will always be non-empty (because the name was either entered by the user, or auto-generated on initial insert).

So... in order to achieve this, we would need a bit of logic to determine if the log's name was auto-generated or user-entered.

This could be accomplished by running the name-generator code using the original entity when the log is saved, and comparing it to the original log name, to see if it matches. If it does, then the log name was auto-generated, and therefore should be auto-generated again when the log is updated.

Remaining tasks

  1. Add logic to check if the log name was auto-generated during initial insert
  2. If it was auto-generated, regenerate it on entity update
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m.stenta created an issue. See original summary.

m.stenta’s picture

Status: Active » Needs review
FileSize
4.52 KB

Initial patch for testing/review.

Status: Needs review » Needs work

The last submitted patch, 2: 3176835-2.patch, failed testing. View results

m.stenta’s picture

Status: Needs work » Needs review
FileSize
5.5 KB

Forgot to update the other tests. Let's see if this fixes them.

Status: Needs review » Needs work

The last submitted patch, 4: 3176835-4.patch, failed testing. View results

m.stenta’s picture

Status: Needs work » Needs review
FileSize
16.07 KB

I think I need to figure out how to mock up timestamp field form submissions. :-)

Try again...

Status: Needs review » Needs work

The last submitted patch, 6: 3176835-6.patch, failed testing. View results

m.stenta’s picture

Ah ha! The tests revealed a flaw in my actual patch code (as they should)!

I was assuming that $entity->original was available in doPostSave() when a log is updated... but this appears to be incorrect.

So my code for comparing to the old generated name is not even running. Will have to figure out how to get the original log in this context...

m.stenta’s picture

Doh! EntityStorageBase->doPostSave() runs unset($entity->original); as it's final step: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

We run parent::doPostSave($entity, $update); right before our name generating logic, so we don't have the original entity. We do this because we need it to run afterwards in the case of new logs so that we can get the Log ID for use in token replacements.

m.stenta’s picture

Status: Needs work » Needs review
FileSize
16.2 KB

This patch adds $original = $entity->original before the call to parent::doPostSave($entity, $update); so that we have the original entity for comparison.

m.stenta’s picture

Oops - includes patches in my patch. This one is clean. :-)

The last submitted patch, 10: 3176835-10.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 11: 3176835-11.patch, failed testing. View results

m.stenta’s picture

Status: Needs work » Needs review

I think there is a timezone issue with my tests... the expected/actual values are exactly 11 hours off. (thinking face emoji)

m.stenta’s picture

Testing based on timestamp/timezone was a silly idea anyway. Here is an updated patch that uses the status field instead.

Status: Needs review » Needs work

The last submitted patch, 15: 3176835-15.patch, failed testing. View results

m.stenta’s picture

Status: Needs work » Needs review
FileSize
5.77 KB

Status: Needs review » Needs work

The last submitted patch, 17: 3176835-17.patch, failed testing. View results

m.stenta’s picture

Status: Needs work » Needs review
FileSize
5.46 KB

Status: Needs review » Needs work

The last submitted patch, 19: 3176835-19.patch, failed testing. View results

m.stenta’s picture

Status: Needs work » Needs review
FileSize
5.72 KB

Ah I understand why that failed. My assumptions about what was happening in testEditLog() were wrong. I assumed $log = $this->createLogEntity(['type' => 'name_pattern']); was creating a log with an auto-generated name, but in fact it is setting the name to $this->randomMachineName().

This is good. It made me rethink the test I wanted to perform, and I added another test in the process. With this updated patch, testEditLog() will test three things:

  • Test that a manually set name does not get overwritten. (This was here before the patch)
  • Test that clearing the name forces it to be auto-generated. (New test)
  • Test that updating a log with an auto-generated name automatically updates the name. (New test (included in previous patches))
m.stenta’s picture

Woo hoo! Tests passed! :-)

Attaching a final patch that restores one of the comments we had in there originally, regarding the fact that parent::doPostSave needs to run before our name generation logic so that new log ID can be used in token replacement. I think this is important to keep for future reference.

m.stenta’s picture

Spoke with @pcambra and he suggested moving the logic above parent::doPostSave() (while keeping the actual token replacement logic below it). This avoids the need to save/clone $entity->original.

Updated patch attached. If this passes I think we're good to merge.

  • m.stenta committed 5f7d39d on 2.x
    Issue #3176835 by m.stenta: Regenerate log names on update using the...
m.stenta’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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