Serial not generating on entity save in certain contexts

@madelyncruz:

Thanks for reporting this issue, and working to fix it.

The usual practice is to assign an issue to yourself when you are working on it, then unassign it when it is ready for review.

I made some suggestions on the MR, but there are some bigger points as well.

First, I think that hook_entity_insert() is the wrong hook to use here. It means an extra save, which might even lead to an infinite loop. We should use hook_entity_create() or hook_entity_presave() instead, both of which are called before the entity is saved. See Entity CRUD, editing, and view hooks.

I suggest using hook_entity_create(). This will be called on new entities, not updated ones. That should be enough: it saves the trouble of checking whether the field is already populated and quitting if it is. Also, there might be some edge cases where the site uses custom code to remove the serial field in special cases. In that case, we should not add it back whenever the entity is updated.

The second point is that a change like this needs to have an automated test to go along with it. The test should do at least the following:

1. Prove that there is really a problem, and that the proposed changes fix that problem. The test should fail without the change and pass with the change.
2. Confirm that there is not an off-by-one error. Especially since the code comes with a comment about subtracting one, we have to be careful about that.
3. Guard against future changes to the codebase. Maybe some day we will think of a simpler way to manage the serial field so that you do not have to subtract one. If that happens, then we have to make sure that this code still works, or the test fails so that we know to update it.

The second and third items can be covered by the same test.

If you are not familiar with writing PHPUnit tests, then at least come up with some lines of code to show the steps. I often use drush php for that sort of thing. Then I can convert it into a test.

The last point is that we may be able to simplify other code in this module once we implement the right hook. (Maybe we can remove code in Drupal\serial\Plugin\Field\FieldType\SerialItem that populates the field.) We might do that as part of this issue or as a follow-up.