[regression] rest_update_8201() can fail and break things

I'm also having this problem, in a fresh 8.2.x install (no upgrade from 8.1.x), with http://cgit.drupalcode.org/migrate_plus/tree/migrate_example_advanced/mi...

Error message: Client error: `GET http://d82.dd:8083/migrate_example_advanced_position?_format=json` resulted in a `404 Not Found` response:
{"message":"No route found for \u0022GET \/migrate_example_advanced_position\u0022"}
 at http://d82.dd:8083/migrate_example_advanced_position?_format=json.

I've also enabled the /dblog/{id} endpoint through restui, and can't get that to work either.

The video shows you had an EntityStorageException that you didn't mention in the issue summary. Could you add that please? @mikeryan: Same or similar exception?

@mikeryan: Oh you didn't upgrade.

That and the fact that #2 wasn't an upgrade...

Because I had already run the upgrade once, the entity already existed, hence the error. T

How can you run the update again? Drupal prevents update functions (to migrate data from one version to the next) to be run multiple times, precisely for this reason.

Are you seeing this error?

#2308745-253: Remove rest.settings.yml, use rest_resource config entities:

I tried the update path from 8.1.3 to 8.2.x via drush. I got the Failed: The "rest_resource_config" entity type does not exist. on the first try. I think we have to rebuild the entity type registry? The update worked flawless on the second drush updb. I did not call drush cr before the first drush updb. Worth a follow-up?

Which was reported again at #2721595-61: Simplify REST configuration:

rest module

Update #8201

Failed: Drupal\Component\Plugin\Exception\PluginNotFoundException: The “rest_resource_config” entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 125 of /Users/george.cassie/Sites/devdesktop/drupal-8.1.6/core/lib/Drupal/Core/Entity/EntityTypeManager.php).

(We forgot to open a follow-up for that.)

It'd be great if you could try to reproduce this. If so, this will become a critical issue, and one that will have to be fixed before 8.2.0 is released.

Oh wait, #2 is indeed not an update. So that can't be the cause.

#2: After deploying the config, did you rebuild your routes?

This happens when you have warm caches when running the update, I just reproduced this when updating to 8.2.0-rc1, I don't think something changes since then.

An explicit call to clearCachedDefinitions() on the entity type manager should fix this before calling getDefinition() should fix this.

Berdir at #10:

An explicit call to clearCachedDefinitions() on the entity type manager should fix this before calling getDefinition() should fix this.

dawehner at #2308745-254: Remove rest.settings.yml, use rest_resource config entities:

+  \Drupal::entityDefinitionUpdateManager()->installEntityType(\Drupal::entityTypeManager()->getDefinition('rest_resource_config'));

This line is the problematic line. Conceptually we should NOT get the definition from the entity type manager, but rather hardcode the definition in the update function which was available at the point in time we wrote the code.
So we better fix the update function I guess. Practically it doesn't matter than much because this is a config entity anyway, which doesn't deal with all that.

Both achieve the same result, but the latter seems the correct one to me. If the rest_resource_config entity type definition changes at a later time, then this particular update function will install the definition as it is at that later time, rather than at the time of this particular update function. Which means subsequent update functions will try to update it to the then-newer version, but it'll already be at that version.

Therefore, I'm going to go with @dawehner's proposal.

P.S.: alexpott raised this very concern at #2308745-245: Remove rest.settings.yml, use rest_resource config entities, but dawehner felt there was no harm because it's just a "create" operation, and I agreed. We were wrong.

This patch takes the config entity type definition in the annotation:

/**
 * Defines a RestResourceConfig configuration entity class.
 *
 * @ConfigEntityType(
 *   id = "rest_resource_config",
 *   label = @Translation("REST resource configuration"),
 *   config_prefix = "resource",
 *   admin_permission = "administer rest resources",
 *   label_callback = "getLabelFromPlugin",
 *   entity_keys = {
 *     "id" = "id"
 *   },
 *   config_export = {
 *     "id",
 *     "plugin_id",
 *     "granularity",
 *     "configuration"
 *   }
 * )
 */
class RestResourceConfig extends ConfigEntityBase implements RestResourceConfigInterface {

… and maps that to a corresponding new ConfigEntityType([…]) instantiation, so that it no longer relies on the entity type manager. Which means no more caches to be cleared (as Berdir says), nor any chance of using not the appropriate version of that definition (as dawehner says).

Works for me. This is pretty much a no-op anyway for config entity. entityDefinitionUpdateManager() doesn't care about them and doesn't do anything for them, but complains if they don't exist.

WFM +1

Theoretically we could have update tests for this, but it'd be a lot of work compared to what is equivalent to "use the literal schema definition instead of hook_schema()". So opened #2803865: Additional test coverage for rest_update_8201() as a follow-up and committed to 8.3.x and 8.2.x, thanks!

Heh, Matt got in while I was adding my comment:

I'm still having my non-upgrade-related issue: #2803905: Custom REST resource no longer accessible after upgrading to Drupal 8.2.x .

So, should I close that one while we take it back up here? Seems the update issue was a red herring...

@MKorostoff: That is a different error. It complains that the entity already exists. That means you already did run the updates before, at least partially. that isn't supported. You have to manually delete those configs or you need to restore a db dump from before you started to update.

Let's discuss Mike's issue in #2803905: Custom REST resource no longer accessible after upgrading to Drupal 8.2.x , the update change here makes sense in itself.

Thanks, Berdir, for responding to #18.

Thanks, catch, for responding to #19.