#no-needs-review-bot

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

Since twig/twig 3.9: Twig node "Drupal\Core\Template\TwigNodeTrans" is not marked as ready for using "yield" instead of "echo"; please make it ready and then flag it with the #[YieldReady] attribute.

Twig deprecations may break contrib tests. Can we backport this fix to Drupal 10.2?

I'd be lying if I said I fully understand the view config update (not one of the 5 haha) but running the test-only feature the tests for the update are failing as expected

1) Drupal\Tests\views\Functional\Update\EntityArgumentUpdateTest::testViewsFieldPluginConversion
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'entity_target_id'
+'numeric'

Can see the full list of expected test failures here https://git.drupalcode.org/issue/drupal-2640994/-/jobs/1604882

So believe the addition of that is good

Wow, what a colossal PITA. 😆 Guess it's good I jumped through all those hoops, since:

1. There were indeed some test views with stale argument plugin definitions that needed to be manually fixed, including the view for testing this new functionality. 😂
2. EntityArgumentUpdateTest was making some slightly bogus assertions.

However, I had to completely rewrite ViewsConfigUpdater::processEntityArgumentUpdate() to operate on entire View entities, instead of using all the per-handler plumbing we've got. To my great dismay, if you copy the existing patterns, ViewsConfigUpdater process functions make a bunch of changes to handler config, then try to save the view, but that instantiates another ViewsConfigUpdater object (this time, with deprecations enabled) to do all the work again. 🤯 So apparently, The Right Way™️ to update a view with all this plumbing is that your process function has to directly update the $view (which isn't even normally passed in to your process function if you think you're just dealing with handlers), not just fix the $handler. 🤦‍♂️

I guess I'm now in the exclusive club of ~5 people on Earth who understand how this class works. 😬 😂

ANYWAY, we're back to green pipelines on 11.x and 10.3.x. I've given up on the 10.2.x MR at this point. It works great for the project where I needed this fixed, all the additional hassles are irrelevant to that site, and this is clearly not getting backported. I just hope this can still land in 10.3.x!

Thanks,
-Derek

I got a bit more clarity from @catch on what's expected here. I pushed some commits to the 11.x and 10.3.x MRs to trigger deprecations when ViewsConfigUpdater::processEntityArgumentUpdate() changes a view outside of post_update. I'm not sure on the wording of the deprecation message, that could use another pair of eyes. Also unclear if we "need" tests that this deprecation plumbing works as expected. I sure hope that's not my responsibility here. 😅

I'll keep an eye on the pipelines to make sure I didn't break anything, but I hope this is now actually RTBC.

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

I asked in #bugsmash for help on this. @catch pointed me to 📌 ckeditor5 and editor module tests rely on hook_editor_presave() bc layers Active , but that issue has nothing to do with ViewsConfigUpdater. I still don't exactly understand what's being asked of me.

Views is doing this:

function views_view_presave(ViewEntityInterface $view) {
  /** @var \Drupal\views\ViewsConfigUpdater $config_updater */
  $config_updater = \Drupal::classResolver(ViewsConfigUpdater::class);
  $config_updater->updateAll($view);
}

@alexpott, are you proposing we should change the API for ViewsConfigUpdater::updateAll() to take a 2nd argument to determine if we're being called from presave or post_update? Or do you propose that views_view_presave() checks the return value from ViewsConfigUpdater::updateAll() and if it ever gets back TRUE it should always trigger a deprecation?

Can we pretty please move all such complications to a dedicated follow-up, and fix this before the 10.3.0-beta1 ships? I still fail to see why this poor bug needs to be the one to deal with all that additional functionality.

Tentatively moving back to RTBC to get more committer eyes on this.

Thanks,
-Derek

Updated description formatting.

So we can add functionality to EntityInterface if we want to.

While we can add new apis, it's better to not add BC breaks if we can avoid it. Similarly to the entity class, we are adding a function to the EntityStorageInterface too, which is also a BC break. We probably want to add an interface that add the create function. It would be nice to see these two new interfaces be removed in a future version of Drupal core (probably not 11 at this point, but 12 maybe?).

Is the pre/postDuplicate stuff necessary?

Sure, technically a hook is all that's needed to cover the basic description, but when I ran into this issue, i also wanted to check the duplicate state at save time, which isn't covered by just using a hook unless I create those properties myself during the hook.

Both the config entity base and content entity base make use of the pre/post hooks. We could move that logic into the storage class, but I would recommend against it. If the api is setting an expectation for what will happen with an entity, then the logic should live on the entity class, even it its more functions. IMO the storage service should deal directly with loading entities, and not with managing entity properties field data. This is the best way to ensure that data is updated correctly over time.

Moving this logic to the entity storage class is a bad idea because a developer could override the storage class with a custom implementation. While we hope that the class override extends the existing base storage, we can't be sure that it will and that means the new class might not include the same logic for updating the entity during these events and for the duplicate operation to work consistently, then entity class is the best place for the logic.

The other question is what does core already do? The entity classes already have pre/postSave() and pre/postDelete() functions, so there is precedence to have a similar api structure for duplicate. Additionally, since the duplicate process is similar to the create process, which would call the entity's constructor, we should have a way for the entity class to react to that as well, no?

On the flip side, there is a lot of logic in the existing content/config entity storage base classes as well. For example both createRevision() and createTranslation() exist on the ContentEntityStorageBase . Given the possibility of overriding the class the field value updates should probably be moved to the entity class instead, unless there is a strong need to tie the logic into the storage service such as loading the new serial id from the database. Given the current logic for these two functions, the main reason we need to use the storage handler for entity operations is to expose the module handler service in order to invoke info and alter hooks. If we want hooks, this may be unavoidable, but the hooks could be run along side.

Given the above, I would still couple any entity data changes as closely to the entity class as possible, and for translation and revision support, that should likely live in the relevant traits instead of the generalized storage.

I am for simplifying logic where possible. Right now during clone the following happens.

1. User calls EntityBase::create()
2. That gods to EntityStorageBase->create()
3. We then call EntityBase::preCreate()
4. Then EntityStorageBase->doCreate() which calls EntityBase::__construct()
5. EntityStorageBase->create() then calls $entity->enforceIsNew()
6. Folloed by
   

    $entity->post create() ?</li>
     <li>Finally the "entity_create" hook</li>
   </ol>
   
   This flow is pretty lean, and doesn't expose any pre/post operations on the storage class itself, so either we would need need to add support for pre/post functions there, or a developer would be forced to override <?php EntityStorageBase->create() 

   to alter the logic, which seems more messy then adding a few extra functions to the process ahead of time. Maybe its not necessary, but seems like its a better Developer experience to provide the apis up front, which is what I think this patch is currently doing to mimic the create flow.

   What is the use case for the getDuplicateSource() and related methods?

   I think the duplicate source is a valid thing to keep track of during the duplication process. Sure this could just be a variable passed to the classes, but I think it makes sense to store it on the entity level as a temporary value, in order for use of the "duplicate" state outside of the hooks. For example would be nice if a module wanted to change something in the form during load, based on the duplicate state, there is no way for it to identify that without the class property/function.

   What's left?

   Skimming the comments, it seems like the last major issue outside of the above discussion is that "there is also no new test coverage for the hooks yet". Is there anything else that still needs to be done here?

MR !1761 needs rebase

catch → closed merge request !8018

catch → closed merge request !8019

Committed/pushed to 11.x/11.0.x/10.4.x/10.3.x respectively, thanks!