entity.last_installed_schema.repository has a race condition

Fixed the phpdoc and the return type.

The patch factors out the uncached getter from the current getter and makes the setter use it instead of the current getter.

Can we get a test case showing the bug

Sorry but no. I do not know how to write a meaningful test case for this. You need multiple processes running in parallel at a very precise point in time.

To recap

1. Field1 imports and deletes the cache
2. Field2 import starts
3. Racing process starts and reads the table
4. Field2 import writes the table
5. Field2 import deletes the cache
6. Racing process writes the pre-field2 state of affairs back into the cache
7. Field3 import reads the cache
8. Field3 import adds field3 to the data structure read from the cache and writes it to the table. It is at this point where field2 gets lost

Following the example of #3312001: Race condition with automatic deploy steps on ConfigImporter I added more comments and I am removing the needs test tag. The enormous difficulty of writing one vs the trivial change here just not worth it.

Except I put the comment before the wrong line... out of two didn't manage to hit the right one. SIgh.

I am noting there's still a race chance here with two field storages being installed at the very same time: field1 reads, fields2 reads, field1 writes, field2 writes but this window is really really tiny against a data structure rarely written. We can decide to protect against this one as well by taking out a lock.

Maybe, it's just much messier core process wise to pass in a new argument especially because this being a bug this will need to be backported all the way to 9.x which has a deprecated parameter pass already and this race is much less likely than the one originally described in the issue. The original race merely requires an ordinary request to race with a field install one, this is between two field installs. But technically, to be super correct, yes. Note this lock doesn't mean the original race is solved with it -- we do not want to add even observing the lock to get, that's an unnecessary slowdown when the original race is solved with this rather simple change to set. Yes, set becomes marginally slower but who cares about set speed.

I really should read my patches before I upload the, not after :)

It totally should be, that's what phpstan was complaining about too.

Y'all understand this drives contributors away, right?

   * @param \Drupal\Core\Lock\LockBackendInterface $lock
   *   The lock backend.

This has negative value. It adds absolutely nothing, the interface already tells you what it is and it makes the class harder to read. I'd rather add a phpcs bypass than this.

But here it is, hoping for the day when we can stop cluttering our patches with this. Is there a policy issue already not to add doxygen if the type already tells all there is?

You know what, this is not a great patch, this is not a visionary solution.

Why are we using a serialized blob here.

If we were writing one database row per field to key-value , none of this would occur.

We could still cache the information together after a single getAll. There's no discussion on this in #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code.

What do you think.

Someone should file an issue to replace the meat of getLastInstalledDefinitions() with

    $this->entityTypeDefinitions = [];
    // Filter out field storage definitions and ensure that the returned array
    // is keyed by the entity type ID.
    foreach ($all_definitions as $key => $definition) {
      $parts = explode('.', $key);
      if (array_pop($parts) === 'entity_type') {
        $this->entityTypeDefinitions[$parts[0]] = $definition;
      }
    }

because IMO it's much easier to read and only iterates the array once.

But anyways.

What about the attached patch. Needs a healthy dose of an update function to unserialize, of course. And maybe still a lock this time in setLastInstalledFieldStorageDefinitions but I am not so worried about that one, all hell already breaks lose if a racing process starts just in time for an entity type to exist but EntityTypeListener has not yet called setLastInstalledFieldStorageDefinitions because then the cache will store empty. I am not even sure that's in scope for this issue.

Quickfix.

Oh yeah, update tests are unhappy.

The first failure is from #2560237: UpdatePathTestBase saves the root user before updates have run but fixing that I found drawing the update page calls hook_theme which calls views_theme which requires a working entity field API. Then there are broken tests which presume entity field API is working, one such is #3364503: BlockContentRemoveConstraint presumes a working entity field API I provided something that makes that one pass.

I really can't emphasize this enough: as far as I am able to tell, all the failures above are not from this patch, it just sheds light on tests which have unfounded trust in the entity field API working with an unupdated database.

Whoever wants to bother with the misery core development became can continue, I am unsubscribing from this issue and until #3364512: [Meta] Run tests with phpcs fail I will submit the absolute minimum possible to make my issues go away and immediately unsubscribe from everything. This process is no longer tenable.