@achap

Can confirm the updated MR #99 is working for webp files. Thank you. I can see the code to handle avif files there as well but I don't use avif so haven't had a chance to test. Will leave some comments on the MR.

I've added an option to the tmgmt_content.settings for this. This should not affect existing use cases but let's see if anything breaks in the tests.

I personally don't use content moderation yet, so I'm not sure how to add this into the UI plugin but I do plan to switch in the coming months from workbench moderation.

achap → created an issue.

I have updated the other MR to re-use the existing manager like this one

After using this hook approach for a few months I've come to decide it is not ideal and the event subscriber is better. It caused a few weird bugs saving it twice and the code required is complex to maintain.

I also found another use case for altering the translation response. Google lets you not translate certain items by wrapping them in Item to translate. which is really useful but you might not want that in your markup, so you can unwrap it once the translation is received.

Both the original issue and that can be solved easily with the event. I also added an additional key to the event for context.

As this is not a bug I'll change it to a feature request. Maybe we can get a 1.1.x-dev branch to target with new features, as I currently have another patch conflicting with this one that I also need. But dispatching an event should be pretty safe for a minor release?

We should close one of these. The other MR has tests which capture the bug but I do like the approach in this one of setting the outdated flag alongside the other metadata field, as in mine I am unnecessarily re-loading the metadata service which is already there.

I think it would be good to have a test regardless as these infinite loops can cause a lot of headaches, both in money and endless revisions.

I'm the author of the MR in #3063378 and yeah it is the same issue causing the infinite loop.

Infinite loop bug has been addressed upstream. https://www.drupal.org/project/search_api/issues/3543230 🐛 Calling clear from updateIndex in a BackendClient can result in an infinite pending task loop Active

Developers will still have to manually delete the task from the search_api_task table if safety mode is active and clearing the index wasn't expected.

If the maintainers aren't feeling the safety mode option on the server backend, another more flexible approach could be dispatching an event e.g. beforeClearIndexEvent with a shouldClear method that developers can configure. That way they can implement their own safety mode, or do whatever they need to do.

This was actually a search_api core bug which has now been addressed upstream.

https://www.drupal.org/project/search_api/issues/3543230 🐛 Calling clear from updateIndex in a BackendClient can result in an infinite pending task loop Active

Not a problem, I've actually been on holiday until Monday anyway so it worked out perfectly. The approach looks correct to me, I was also thinking of using a static variable to keep track of the currently active tasks but I'm not that familiar with the core search_api code and just checking if there is any active task seems simpler/better.

I also tested it alongside my patch in the related issue and it worked perfectly. No more infinite loop and the task completes successfully.

Vielen dank!

achap → created an issue.

• Added the safety_mode option to the server backend.
• Added logging to the mappingsHaveDifferences method so we know why something is different.
• If getting mappings doesn't succeed for some reason throw an exception.
• Removed a conditional from mappingsHaveDifferences which I think was redundant.
• Throw an exception if safety mode is activated

I also noticed that when a task is created and you try to re-run it (after disabling safety mode) there is an infinite loop because when the task is retried updateIndex calls $index->clear() which fires off an event to execute and tasks, which calls updateIndex, which calls $index->clear() etc. I think that's a search_api bug though.

So the developer will have to manually delete the search_api_task.

achap → created an issue.

I ran the query that getMostRecentItem calls as a raw mariadb query and it's pretty quick on its own so I don't think it can really be optimized anymore. The bigger problem is that it's called every time addItem is called on the continuous manager and I don't think entity queries get cached out of the box (could be wrong on that), so only executing it when necessary is already a big improvement. There's probably other things we could do like statically caching the result for the duration of the request but that might be overkill.

OP mentions the dispatcher is the bottleneck but when I look at the profile even though it gets called 168 times in my case, only 19ms is exclusive wall time. 4500 calls is a lot. 168 calls is per entity, so if when you save the user profile you are triggering updates to other entities then that could quickly add up. If fixing getMostRecentItem doesn't help, most likely something in your event subscriber is the bottleneck. E.g. maybe there's a database or network call happening there. I do use the entity manager to load entities in my subscribers, but I believe they get cached by default whereas it doesn't look like entity queries do.

Some performance stats.

Tested on local machine, caching enabled and xdebug disabled.

Saving an entity that isn't connected to a continuous job:

Before patch: 168 calls to getMostRecentItem and 4.73 seconds of exclusive wall time in the subsequent database query. Total time 6.96 seconds.
After patch: It is never called. Total time 1.92 seconds.

Creating a new entity that is connected to a continuous job.

Before patch: 168 calls to getMostRecentItem and 4.68 seconds of exclusive wall time in the subsequent database query. Total time 6.5 seconds.
After patch: 4 calls to getMostRecentItem (1 per job it is linked to). 230 ms of exclusive wall time. Total time 1.89 seconds.

Marking an entity that is already part of a job as outdated

Before patch: 168 calls to getMostRecentItem and 2.8 seconds of exclusive wall time in the subsequent database query. Total time 5.3 seconds.
After patch: 4 calls to getMostRecentItem (1 per job it is linked to). 23 ms of exclusive wall time. Total time 2.27 seconds.

There could be more going on for OP but I think this the main bottleneck for us so going to mark for review and use this patch.

I found that moving the call to getMostRecentItems improves things a lot if you just move it after the should create check so I added it to a MR. Even saving a page that is already translated and not marked as outdated was triggering this query.

Switching to a database query did not provide much improvement though but I will do more investigation because it is still slow when this query does get triggered.

I think we need both addItem calls per https://www.drupal.org/project/tmgmt/issues/2671894#comment-10927193 → so I don't think we should mess with that. I know entity queries have performance issues and there's currently a longstanding core issue to fix that, so maybe we can convert that directly to a database query and see if that helps. Also I notice getMostRecentItem is called before we even check if we should create an item so that should definitely help limit the database calls.

Looks like the second addItem was introduced here:

https://www.drupal.org/project/tmgmt/issues/2671894 →

I'm also experiencing the same performance issues. Can we just double check the terminology in the IS though. You say you have 4500 jobs but I think you mean 4500 job items?

I have structured my site to have one continuous job containing one node type. So per language I could have something like 10 continuous jobs. When a node is updated it will loop through every job. So if you have 10 jobs and 5 languages it can quickly add up. One possible improvement is to structure it so that you have one continuous job per entity type and include all the bundles in that job.

I have a blackfire call graph which shows that the majority of the time (7.5 seconds) is spent in IO waiting for the execute triggered by getMostRecentItems:

I guess the fix would be to try and trigger it less often and also optimize that query. In tmgmt_content_entity_update addItem is called twice, once in that function itself and also below when it calls tmgmt_content_create_continuous_job_items

Thank you for this patch. It fixes this issue, as well as another issue I was having over in https://www.drupal.org/project/drupal/issues/3025039#comment-16046212 🐛 New non translatable field on translatable content throws error Needs work with content_translation_uid field changing for programmatically created taxonomy terms which are then edited via the UI.

This issue can also occur when you programmatically create taxonomy terms and then translate them programmatically too, because the content_translation_uid field is NULL in the database by default. When you go to re-save the previously created translation in the UI you get the above error message because it defaults to the anonymous user (0) when assigning a value via the form.

Added a new commit to MR 120 that fixes an issue where if an entity makes use of the "Hide non translatable fields on translation forms" the translatable fields are hidden even when creating a new node via IEF on a translation of the parent entity.

Since the rebase in #86 there were some upstream changes to PHPStan. Notably $height and $width were removed from the baseline and return types were enforced for all new methods which caused an error with the trait we were using:

https://www.drupal.org/project/drupal/issues/3461318 ✨ Enforce return types in all new methods and functions Active
https://www.drupal.org/project/drupal/issues/3347502 📌 Fix PHPStan L1 errors "Access to an undefined property Foo\Bar::$baz" Active

I removed references to $this->height and $this->width and replaced them with $this->get('width')->getValue() and $this->get('height')->getValue() like they did in the related core issue.

Also I added the trait method to the .phpstan-baseline.php like has been done for other tests using the trait. Maybe it would be better if we added a void return type to the ContentModerationTestTrait and generated a new baseline since it's test code rather than runtime code?

Also, the IS is out of date since a new ImageFieldItemList was removed in #74 so I removed it from the issue summary too.

I did a bit more digging on this and I'm fairly confident that default Drupal behavior doesn't automatically mark a translation as unoutdated when you save a translation so I think my fix is correct. I also found a corresponding issue on the 7.x branch that had the same problem and was committed: https://www.drupal.org/project/tmgmt/issues/1619786 →

As for my earlier statement that "The German node was previously being marked as unoutdated correctly." after checking again it definitely wasn't reset to 0 automatically but the issue definitely only starts to occur once you have 2 languages in 2 jobs affecting the same node type. Hence why I added a second Spanish job to the test.

Honestly I think this should be marked as major, as with this bug a new revision will be created on each cron run and it will also be sent for translation continuously costing money.

I guess not a lot of people are using this functionality and instead are just re-requesting the translation via the UI when needed but this is an important feature for me.

I created a merge request as the patch was still creating the revision table even if its not revisionable. Also removed the comments as suggested. Don't know how to test this one off the top of my head but manually testing of both a non revisionable and revisionable entity successfully added + deleted the indexes.

Leaving as needs work for tests.

achap → made their first commit to this issue’s fork.

Explicitly marking the translation as outdated when it is saved in ContentEntitySource means the test passes but I would have thought that would have been done automatically by Drupal? I'm not sure if this is the correct fix but it does stop the loop. I would appreciate if someone could review it and share their thoughts especially if they can figure out why this only occurs with a second job.

When adding an additional continuous job in Spanish and duplicating the steps that were created in German I can replicate it in a test:

    1)
    Drupal\Tests\tmgmt_content\Kernel\ContentEntitySourceUnitTest::testContinuousJobItems
    No continuous job item is automatically created when translation is
    accepted.
    Failed asserting that 4 matches expected 3.
    
    /builds/issue/tmgmt-3063378/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95
    /builds/issue/tmgmt-3063378/sources/content/tests/src/Kernel/ContentEntitySourceUnitTest.php:834
    /builds/issue/tmgmt-3063378/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

As best I can tell, when the translation is accepted and saved, tmgmt_content_entity_update() is called, at which point the untranslated entity is again sent to the continuous manager but at this point in shouldCreateContinuousItem the metadata returns that both of the translations are still outdated. For whatever reason, this doesn't occur with a single translation job. The German node was previously being marked as outdated correctly.

Creating a test to try and highlight the bug

Hi @usingsession,

I tried to reproduce locally on core 10.4 but I couldn't again in my case it appends the submit handler rather than replacing it... however given that two people have said this is happening I will take your word for it and I moved the attach method so it's only triggered if an IEF is present. I'm not sure why that was there but I did base this patch on #95. Simply removing that line caused a test fail, but locally it passed by moving it up into the check. Are you able to check if it's working for you now?

I also updated the MR to include a check for instances where the form object isn't an instance of EntityFormInterface like in #142 with layout builder. That stops the error but given that the subsequent code doesn't execute it might not work correctly. I don't use layout builder though, so not sure. This part might still need some work.

achap → made their first commit to this issue’s fork.

Thanks for your work on the POC. It looks good! We are currently doing a very similar thing in Drupal where we set one of the indexes to be read via an alias and then re-index in the background but as mentioned it's very slow with a large data set as search API goes one by one. So if the re-index was much faster that would be a big win.

What we currently do is:

• 2 indexes in Drupal (A and B).
• A custom form to set the current read index via an alias.
• A clone button to copy field settings from A to B and vice versa.
• If there are changes we clone the current read index, make the changes on the other index, deploy and re-index in production. Then we do another deployment to update the read index alias when everything has been indexed on the new index.

Some thoughts on the POC:

• Initially I had some questions on whether we actually want to copy the tracking data from green to blue in Drupal so it doesn't start from scratch, but actually I realized if new mappings are created then you always want to start from the beginning.
• https://docs.opensearch.org/docs/2.19/im-plugin/reindex-data/#destinatio... There is a slice parameter that might be useful to speed up the re-index operation using parallelism.
• Depending on how long the operation takes maybe it would be too slow to do as part of a deployment? I guess we'd need to test it on a large dataset. Ours is currently about 100,000 documents which is reasonably big but there's probably people out there using much larger ones. Maybe it would be better to schedule the re-indexing as a background task (I think that is what search_api_task is for?) and then provide a simple drush command to do it? The drush command could then update the alias once it's finished or there could be a form to do it when the user is ready. Maybe this could also be a configurable option so that people with smaller data sets can do it all in one deployment?
• When we close the index no longer in use, it might be useful to set it to read-only in Drupal to prevent further indexing, especially if integrations are set up to things like OpenAI which costs $$$. I guess closing it in OS would prevent writes but there might be errors on the Drupal side if it's still trying to call. We currently do this manually in our solution but sometimes forget so if it was automated that would be nice.
• I guess some consideration would need to be given to existing set ups like ours as I guess my A and B indexes would both get green abd blue suffixes, though doing it in a new major release would probably be enough for that case.

I'm also interested in this. It looks like someone has created a parallel tracker exactly like that described in #2.

So I think this is actually a bug and not just with moderated entities. Initially I thought it was due to the referenced core bug (which is why marking revisions as outdated was hidden in the first place in core for moderated entities) but I was actually able to reproduce it for unmoderated entities too on a freshly installed site without any of my custom code.

Steps to reproduce:

1. Setup a fresh ddev instance of Drupal https://ddev.readthedocs.io/en/latest/users/quickstart/#drupal
2. Require tmgmt with composer and install via drush.
3. Enable the tmgmt_test module via drush so the test translator is available.
4. Enable German as a language.
5. Enable Spanish as a language
6. Make the default article bundle translatable along with all fields
7. Setup a continuous job for the article in German using test translator
8. Setup a continuous job for the article in Spanish using test translator
9. Create an article and save it
10. Run the following SQL and you should see two inactive continuous jobs select tji.item_id, tji.tjiid, tj.target_language from tmgmt_job_item as tji join tmgmt_job as tj on tji.tjid = tj.tjid where tj.job_type = 'continuous' and tji.state = 'inactive';
11. Run ddev drush php-eval "tmgmt_cron();" and then run the SQL above again and there should be no inactive job items.
12. Go to the article in the original language and click mark translation as outdated then save it.
13. Now if you run the SQL again there should be two inactive items
14. Now run ddev drush php-eval "tmgmt_cron();" again and when you run your SQL you will see one job item remaining.

Now matter how many times you run tmgmt_cron() via drush from there there is always one remaining inactive item and it alternates between de and es. It's like one is always marking the other as outdated.

Would appreciate if someone else can confirm my steps to reproduce. Would potentially think about marking this as major if it's confirmed as this can cause a huge amount of revisions/costs from re translation.

Versions: Drupal core 11.1.8, tmgmt 1.17.0

achap → created an issue.

Attaching a patch from the MR for composer

achap → created an issue.

One possible idea is to append the link and title client side. This could be deduced from the url and tag?

achap → created an issue.

Thanks for the steps to reproduce @tostinni. I just tried toggling the translatability of the Article title field but it was correctly saved. I'm using IEF 3.0.0-rc20 and Drupal core 10.3.14. Might be a version thing or another conflicting module? Hard to say. If other people could test that would be great.

@undersound3 #138 What were you doing when that issue occurred. Does the parent node or the inline entity you're trying to translate not have a source language set? i.e. und. There are quite a few core/contrib issues about the same thing so it might be something that needs to be accounted for

achap → created an issue.

If I understand correctly with both MRs it will only solve the problem when directly saving a menu item which is not that common. You will still need the contrib module: https://www.drupal.org/project/menu_cache → to prevent cache invalidations of menus using that node, so in that case changing the return type like in my MR doesn't seem worth the risk of potentially breaking something.

So yeah we should consolidate on the other one but people should be aware though that other modules can also trigger a menu save (see #13) e.g. token, custom implementations.

I will close the other MR.

I'm using the patch for 10.3 from number #80 and it's working for me in that the error goes away. I tried re-running the tests again to see if it was a random fail as it didn't seem related but I got more failures. E.g.

Drupal\Tests\migrate\Functional\MigrateNoMigrateDrupalTest::testExecutionNoMigrateDrupal
Behat\Mink\Exception\ResponseTextException: The text "Migration was
successful." was not found anywhere in the text of the current page.

Is failing because of this:

Drupal\Component\Plugin\Exception\PluginException: Plugin (d6_field_formatter_settings) instance class "Drupal\migrate_drupal\Plugin\migrate\FieldMigration" does not exist. in Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (line 97 of core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php). 

At least that one doesn't seem related to this change. Not sure about the others.

Uploading patch from the MR. Currently the only way to retry the jobs is to write an update hook to reset the job state.

achap → created an issue.

I realized it's actually possible to do this without an event already using hook_tmgmt_job_after_request_translation() however it's not as easy and results in saving the Job item and entity twice. But I want to have a backup if the MR isn't committed.

You can do it like this:

/**
 * Implements hook_tmgmt_job_after_request_translation().
 */
function my_module_tmgmt_job_after_request_translation(array $job_items) {

  foreach ($job_items as $job_item) {
    // Only start working if the previous job was accepted.
    if ($job_item->isAccepted()) {
      /** @var \Drupal\tmgmt\Data $data_service */
      $data_service = \Drupal::service('tmgmt.data');
      $unfiltered_data = $job_item->getData();
      $data_items = $data_service->filterTranslatable($unfiltered_data);
      $changes = FALSE;
      foreach ($data_items as $data_item_key => $data_item_value) {
        // Issue with data-entity-embed-display-settings encoding
        // post translation.
        $original = $data_items[$data_item_key]['#translation']['#text'];
        $result =
        // @see https://www.drupal.org/project/tmgmt_google_v3/issues/3508188
        preg_replace_callback(
          '/data-entity-embed-display-settings="(\{[^<&]*?})"/',
          function ($matches) {
            $json = $matches[1];

            // Decode JSON.
            $decodedJson = json_decode($json, TRUE);
            if ($decodedJson === NULL) {
              // Return original if JSON decoding fails.
              return $matches[0];
            }

            // Re-encode and escape JSON for HTML attributes.
            $encodedJson = htmlspecialchars(json_encode($decodedJson), ENT_QUOTES, 'UTF-8');
            return 'data-entity-embed-display-settings="' . $encodedJson . '"';
          },
          $data_items[$data_item_key]['#translation']['#text'],
        );
        if ($result !== $original) {
          $changes = TRUE;
          $data_items[$data_item_key]['#text'] = $result;
        }
      }

      // The job item must be re-marked as active for the data to be propagated
      // to the node, otherwise it will only be saved on the job item (not the
      // corresponding entity) as the translator already marks it active. Only
      // mark it as active if something actually changed to prevent unnecessary
      // saves.
      if ($changes) {
        $job_item->active();
        $job_item->addTranslatedData($data_service->unflatten($data_items));
      }
    }
  }

}

The event would be nicer but up to you if you want to include it!

Also, I should add the reason my workaround of using a filter doesn't work is because when you go to save the translation in the UI, the filter doesn't get applied. Also not in the translation review section... so that solution was not working properly.

As I couldn't think of a way to solve this without hardcoding a dependency on entity_embed what about dispatching a post translation event directly after the translation is received and the response is decoded. This fixes my problem. Uploaded patch based on merge request.

This issue can also occur when you programmatically create taxonomy terms and then translate them programmatically too, because the content_translation_uid field is NULL in the database by default. When you go to re-save the previously created translation in the UI you get the above error message because it defaults to the anonymous user (0) when assigning a value via the form.

Patch from #128 works for me and the content_translation_uid is still correctly assigned.

Ok so I was able to dig a bit deeper into it and the error I posted above in #2 is not related so can be ignored. I stepped through with my debugger after clicking "Execute Pending Tasks" and in updateIndex it is calling clear because there are some differences between the opensearch mappings and the drupal mappings, however what appears to be happening is deleting the index is causing search api to dispatch the updateIndex event again before it actually deletes the index, but because we haven't actually cleared the opensearch index the mappings are actually still different, so it's causing a redirect loop where deleting just continually calls updateIndex. I've posted some screenshots from my debugger.

Stack trace showing the redirect loop:

For some reason search api is executing all those tasks:

For more context, this error is in the logs: OpenSearch\Exception\ForbiddenHttpException: [] in OpenSearch\Exception\HttpExceptionFactory::create() (line 25 of /app/vendor/opensearch-project/opensearch-php/src/OpenSearch/Exception/HttpExceptionFactory.php).

However, once truncating the table I can index fine so I don't think it is an authentication issue.

achap → created an issue.

I think Html::decodeEntities is already working, the problem is when I look at the original node that has the embed on it (the one that hasn't been translated) it actually stores the embed with the Html entities in tact like:

<drupal-entity data-entity-type="media" data-entity-uuid="bdf34a77-8019-41f6-8234-9ed11370e3cf" data-embed-button="image_bundle" data-entity-embed-display="entity_reference:entity_reference_entity_view" data-entity-embed-display-settings="{&quot;view_mode&quot;:&quot;gallery_full_width_no_thumbnail&quot;}"></drupal-entity>

That doesn't get stripped by Xss::filter on output but the decoded one does:

<drupal-entity data-entity-type="media" data-entity-uuid="bdf34a77-8019-41f6-8234-9ed11370e3cf" data-embed-button="image_bundle" data-entity-embed-display="entity_reference:entity_reference_entity_view" data-entity-embed-display-settings="{"view_mode":"gallery_full_width_no_thumbnail"}"></drupal-entity>

@jksloan2974 do you mean the patch from #131. Do you have any steps to reproduce? I think if you were to apply that patch to a site that was using the previous method from an earlier patch (where the language of the entities was changed rather than translated) I could see that happening but not sure how it would occur on a fresh site.

Looking at PathTranslationTest if I understand the logic correctly:

* When the node is not translatable the path alias is und by default.
* When it is translatable and the path is translatable the default alias gets en and the translation alias gets fr.
* When it is translatable but the path is not, both aliases get und.

Does it make sense to create a second alias with language und in the third case for the fr translation? It seems redundant, since it should just fall back to the en nodes alias. You now have two und aliases pointing to the same node. In that case the test shouldn't populate the path field for the translation, since it should be null.

achap → created an issue.

achap → created an issue.

I re-opened an old issue that was closed in favour of this one which is still happening in 10.3. Interface translations are overriding config translations whenever interface translations are updated . Maybe best to concentrate efforts there if you are experiencing the same?

Interface translations override config translations on update 🐛 Interface translations override config translations on update Active

I think this should be re-opened. A number of people in the referenced ticket are still experiencing the issue described even after the fix. It is happening to me in the following scenario on Drupal 10.3

* I have some country names translated in locale E.g. United States = アメリカ in Japanese.
* We display country names in a language selector. That information is stored in simple config.
* Feedback from client was that country names should be displayed in their own language in the language selector. Translations should still exist in other contexts though.
* Put United States for the translation of United States in the Japanese config for the language selector. You could even delete the whole config file.
* Re-import locale like so: drush locale:import --type customized --override customized ja /app/translations/ja.po
* Export your config. You will notice that the config has been overridden with アメリカ

Just resolved the merge conflict by merging in the latest stuff from 2.x so I could test it out as the patch wouldn't apply.

achap → made their first commit to this issue’s fork.

Added a note that using eval sourcemaps and Drupal.t with webpack does not work properly!

One more update on #129, I have a use case that on certain languages that are English language variants e.g. en-GB and translate from a default en node, that the IE shouldn't be translated to en-GB by default. Rather than translations, they are localizations. Currently a new translation would be copied from the en inline node, and the languages would split, so it's not possible to share content across languages.

In this case, en-GB should still be able to reference en nodes without translating them. There could be other use cases, so the most flexible way is to introduce a new ShouldTranslateEvent that gets dispatched just before translation but keep the behavior described in #129.

Thanks, this works to stop the deprecation warnings in CI but still needs work because there are unused use statements that need to be removed, test code needs to be updated to not rely on the exceptions and there are inconsistencies in the case of NULL, Null etc.

Also, I guess as long as MigrateSkipProcessException is not removed from core this can't yet be removed from the module.

This just occurred for me after upgrading to Drupal core 10.3 but I am on config_split 1.9.0 and config_filter 1.12.0 unlike the others in the issue. All my pipelines started failing. Re-running cim fixed it but I've never had to do that before. So something must have changed in 10.3 that no longer works with the versions I'm running.

In terms of not saving the link in the first place from the node form, I just did a quick debug on my own site and noticed that MenuTreeStorage::save is triggered by 3 modules. First is menu_ui, the other is token module and the third is our own custom submit handler. So even if we preventing saving in menu_ui, a lot of people use token so it would still occur for them.

Is something like what @andy_w posted feasible to do in core for sites that don't have complex access control?

I added a test case to show that this is happening without changing anything else, and addressed some feedback on MR. My use case is pretty simple because we don't have complex access controls like those mentioned in the slack thread. I'm not sure I fully understand the implications described there but I guess it would be good to solve this for anonymous users, while not breaking it for those more complex access scenarios if that's possible.

In my case it happens because of the "Menu Settings" tab on node edit forms. If you click "Provide a menu link" for example and make that node part of a menu, on subsequent saves of the node the menu cache tag gets invalidated even if nothing about the node changed. Seems to happen when directly editing the menu too. That might be unique to my setup but I had a colleague confirm on a different site.

I guess the best thing to do would be write a test proving that that is indeed the case. I'll try and find some time this week to do that.

achap → created an issue.

Updated my fork to handle duplicate aliases caused by altering path alias languages.

Also, I feel like this is actually a bug report, because it's impossible to create a new alias in a different language without overriding the fallback alias. The title could probably be updated too but I'll leave that alone for now.

Hmmm both my MR and patch #10 will result in duplicate aliases being created every time you save a node if you do something like this in hook_pathauto_alias_alter

if ($context['language'] === \Drupal::languageManager()
      ->getDefaultLanguage()
      ->getId()) {
      $context['language'] = LanguageInterface::LANGCODE_NOT_SPECIFIED;
    }

I ran into a small edge case where I have url.prefixes defined in language.negotiation.yml but I haven't yet enabled url detection and selection, so I added a check to make sure the prefix actually exists before the comparison takes place.

I'm happy to write tests, however before I do maybe someone familiar with the menu system can review the approach and see if this is a sensible change?

Thanks for the patch @kasperg. I tested locally running both CLI and web requests and it works as expected.

I'm getting "MySQL server has gone away errors" after long running drush operations like search api indexing and running migrations with the lateruntime processor enabled. Those operations can put thousands of cache tags in the queue and it seems like it is trying to process many of them at the end of the request, or at the end of batches which is resulting in a packet size greater than max_allowed_packet. My max_allowed_packet setting is already quite high and I don't have much room to increase so I agree with the other comments that it would be better to at least have the option to disable it for CLI. Thanks for the MR @Kasperg

achap → created an issue.

Thanks for creating the module! I added glossary support!

achap → created an issue.

All the tests now seem to pass again except for this fail on the phpunit (next major) pipeline:

    1)
    Drupal\Tests\redirect\FunctionalJavascript\RedirectJavascriptTest::testRedirectUI
    Behat\Mink\Exception\ExpectationException: The string "Are you sure you
    want to delete the URL redirect from /web/non-existing?key=value to
    /web/node?" was not found anywhere in the HTML response of the current
    page.

however looking at the pipelines from other commits it looks like its unrelated to my code.

Ran the test-only changes to confirm the test catches the bug. Looks like fixing this has caused knock on effects to other tests though. I'll look into it.

achap → created an issue.

Closing this as it can be prevented by altering the langcode in hook_pathauto_alias_alter rather than hook_path_alias_presave. Pre save occurs too late but the other one is fired before we save the alias so it prevents the duplicate issue.

Also, I guess that the core path_alias should be the one preventing duplicates being saved.

Looks like this patch was on the right path from #95 and then veered away from it. I strongly agree with #95 that we should be translating entities rather than changing their langcode as that is destructive and would confuse editors. I believe the fix in #52 might have been trying to address that but it may be redundant now. Also, it only prevents deleting entities used by the parent node but that could mean they are still deleted if referenced by other entities. Heavily based this MR off #95.

Changes in MR 120:

1) Inline entities are translated rather than having their language changed.
2) Re-attached the submit handler from #95 that was removed and handled the case where the main entity form is submitted but the inline entity form hasn't been opened yet.
3) Removed the deletion code that was trying to prevent entities being removed if referenced by the parent entity (I think that is probably better handled by something like entity_reference_integrity module). See above for reasons.
4) Translations get their own created/updated timestamps when they are created rather than inheriting from the default translation. This helps them show up in the /admin/content view when the parent page is translated.
5) Rename the testTranslation test to testSymmetricTranslation and added cases for translations and timestamps. Also created a new testAsymmetricTranslation test to test asymmetric behaviors which was more thorough than the previous behaviour.

Feedback welcome.

Regarding the comment in #127 the line number you indicated the error on doesn't match up with my MR so I don't think its related. Anyway I'm working on MR 120 now.

I thought this was an issue with the module more generally but turns it out it only happens when using the asymmetric translations patch so will close this and look into it over there.

achap → created an issue.

Just to add some further steps to reproduce. You need uncheck "Include the current page as a segment in the breadcrumb" and "Use the real page title when available" which are turned on by default. Managed to reproduce it on a fresh simplytest.me install

achap → created an issue.

achap → created an issue.

achap → created an issue.

The latest commit of https://git.drupalcode.org/issue/inline_entity_form-2822764/-/tree/2822764-support-adding-new-entities-when-translating applies cleanly to 3.0.0-rc20 as a patch. I made a small change to remove @label from the translatable markup that was never actually getting replaced.

The failing TranslationTest needs to be fixed and it seems to me that this patch has unintentionally changed the behavior even when allow asymmetric translations hasn't been enabled as the test fails on line 108 which is well before we enable it.

    // Both inline nodes should now be in English.
    $first_inline_node = $this->drupalGetNodeByTitle('Kann ein Känguru höher als ein Haus springen?');
    $second_inline_node = $this->drupalGetNodeByTitle('Can a kangaroo jump higher than a house?');
    $this->assertSame('en', $first_inline_node->get('langcode')->value, 'The first inline entity has the correct langcode.');
    $this->assertEquals('en', $second_inline_node->get('langcode')->value, 'The second inline entity has the correct langcode.');