🇪🇸Spain @vidorado

No worries @penyaskito 😁 you're not the only one who has been misleaded by unresolved threads. I don't really know what it depends on. Sometimes I see the "Resolve thread" button at GitLab, but sometimes not.

Since the changes look good to me and the tests pass, I believe it's safe to switch back to RTBC.

Thanks @godotislate!

Done! :)

I've created a CR :)

Done! :)

The comments look good to me :)

Thanks for rebasing, @dcam!

Yes, I agree. We could use another opinion about the "official" and current way of resolving this, since I could have based my code on old and now-not-desirable code.

Rebased again

Updated again

The current solution is actually triggering a dreprecation! Check this

It's ready to review :)

The Needs Review Queue Bot is just saying that this MR needs a rebase. Also a product manager must review this feature, and that has not yet happened.

Updated the MR

It seems that props.properties wasn't being converted to stdClass in the $definition_object used to validate. It was being done for the original $schema var, but not for $definition_object.

All test failures were related to this, and now they pass :)

Feel free to set thiss issue to "Needs Review" if you consider that the work is now done.

Rebased the MR :)

About Berdir's comments above

I believe they have been taken into account. We now understand that we can only add a callback function to a bundle definition to restrict the options or customize the labels, but the allowed_values setting must not be overridden.

Regarding this, I think the only unanswered comment is #11, which asks what would happen if we did override it.

I tested this scenario, and it turns out that it is possible. I created a custom test entity with an options_field that has a default allowed_values setting of ['Foo', 'Bar'], two bundles, and an overridden allowed_values setting of ['Baz'] for the second bundle. I was able to create entities for both bundles, and the values in the options_values field dropdown were displayed as configured, with no errors thrown.

I'm not sure what action, if any, we should take regarding this.

About adding the entity ID to the cache ID

This is a separate question, and I assume you're suggesting adding the entity ID to the cache key because the allowed_values callback function receives the entity as a parameter, meaning it could depend on any part of the entity, not just the bundle.

In that case, I agree that we should add the entity ID to the cache key. It's unfortunate, as this will result in a large number of cache entries—one per entity—but I don't see another way of handling it since the callback function does not return any cacheability metadata.

Please confirm that we're all on the same page, regarding to this, and I'll proceed with adding the entity ID to the cache key.

Thanks!

About Berdir's comments above

I believe they have been taken into account. We now understand that we can only add a callback function to a bundle definition to restrict the options or customize the labels, but the allowed_values setting must not be overridden.

Regarding this, I think the only unanswered comment is #11, which asks what would happen if we did override it.

I tested this scenario, and it turns out that it is possible. I created a custom test entity with an options_field that has a default allowed_values setting of ['Foo', 'Bar'], two bundles, and an overridden allowed_values setting of ['Baz'] for the second bundle. I was able to create entities for both bundles, and the values in the options_values field dropdown were displayed as configured, with no errors thrown.

I'm not sure what action, if any, we should take regarding this.

About adding the entity ID to the cache ID

This is a separate question, and I assume you're suggesting adding the entity ID to the cache key because the allowed_values callback function receives the entity as a parameter, meaning it could depend on any part of the entity, not just the bundle.

In that case, I agree that we should add the entity ID to the cache key. It's unfortunate, as this will result in a large number of cache entries—one per entity—but I don't see another way of handling it since the callback function does not return any cacheability metadata.

Please confirm that we're all on the same page, regarding to this, and I'll proceed with adding the entity ID to the cache key.

Thanks!

After digging deeper into the code, I realized that we were relying solely on the development settings configuration form, but not on the development.services.yml file configuration. This initially puzzled me when I tried to reproduce the issue from #18.

So, I've updated the code to rely on the container's twig.config parameter, ensuring that we honor the actual configured value in both cases.

Regarding the WSOD reported also in #18, I've also set the $this->definitions static cache in ComponentPluginManager::getDefinitions(). Additionally, I updated ComponentPluginManagerTest to modify the container's twig.config parameter, to rely on a more realistic behavior—checking whether definitions are not refreshed when they change, rather than depending on whether the cache backend is called or not.

About we could have catched the bug at first...

I'm not sure what test should be added:

1. A new test in DevelopmentSettingsFormTest that renders an SDC component in debug mode (this seems odd, as it would make Development Settings aware of SDC).
2. A new test in OliveroTest that renders an SDC component in debug mode (makes more sense but feels disconnected from ComponentPluginManager, potentially hiding the reason the test is needed).
3. A new ComponentPluginManagerRegressionTest class to explicitly ensure this issue doesn't happen again (this seems like the best approach to me).

vidorado → changed the visibility of the branch 11.x to hidden.

Thanks for filing an issue @m4olivei, I was wondering if that was the correct approach, rather than re-opening this one. But since this has been reverted and changed to NW, I will work on it.

I will also try to figure out how we could have prevented this bug to be overlooked and avoid it in the future.

Whoops! I will work into this tomorrow ASAP.

Thanks for clarifying this, @catch 😊

Fixed! Restoring to RTBC as it was only adding a docblock to a method.

I can't figure out why the test-only changes test is passing. I've checked the job ouput and everything appears to be correct.

In my local environment it fails as expected when I manually revert the changes made on LocalStream.

I will leave this as "Needs review" so a reviewer can check this on their local environment.

Rebased the MR.

Now it doesn't fail! Haha. In my local environment the test fails as expected when I undo the fix.

Failed asserting that directory "public://foo/custom-dir" does not exist.
/var/www/html/core/tests/Drupal/KernelTests/Core/File/StreamWrapperCustomRootDirectoryTest.php:19

Time: 00:01.228, Memory: 6.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\File\StreamWrapperCustomRootDirectoryTest::testRealPathHandlesCustomRootDirectory
Failed asserting that directory "public://foo/custom-dir" does not exist.

/var/www/html/core/tests/Drupal/KernelTests/Core/File/StreamWrapperCustomRootDirectoryTest.php:19

I will give it a look.

Whoops, that was an unchecked edge case. we must not call \stat() directly with the result of \realpath() because it could return FALSE.

I've added a guard for it.

You're right @kim.pepper, I reflected that with a new title but not also in the IS :)

I've tried to explain it better in the IS.

@vasike, some tests fail randomly, so it's common practice to manually re-run those that fail. Most of the time, they pass on the second attempt.

In this case, the test kept failing, and I remembered having the same error with a CSS stylesheet larger than 90kB in another issue. I think I rebased to make it work, so I rebased the issue fork branch onto the latest 11.x commit. Now it passes 🙂 (I've had to re-run Nightwatch tests, which have failed on the first attempt)

vidorado → changed the visibility of the branch 11.x to hidden.

I've managed to test it 🙂

I believe this test should be in a separate file due to the override of the stat() and realpath() methods, which are closely tied to this new specific test method.

What I'm not entirely sure about is whether this should be done as a regression test, like RegressionTest or JsonApiRegressionTest, or if the current naming and file placement are already correct.

Woops, I mistakenly thought that this was neccessary, like we did in 🐛 EntityCreateAccessCheck needs to enforce a bundle is set by the route for entity types that support bundles Needs work and 📌 Remove NumberTest::testAlphadecimalToIntReturnsZeroWithNullAndEmptyString() test Active .

Was it correct in that case because it was a test removal and not a codebase removal?

That's a very valuable discovery, @claw! 🙂

Now I see that this can be an issue, even if the constraint of the path being relative isn't violated.

Thanks for your input, I will try to make a test that covers the fix.

vidorado → created an issue.

No worries! The search for a good and elegant solution often requires a few iterations. 🙂

I think I searched for other uses of EntityCacheTagsTestBase::getRenderCacheBackend() and couldn't find any, but I overlooked the possibility that contrib modules might be extending this class as well, so I think it's a good idea to keep the method and deprecate it.

I've fixed the naming as you suggested, restored the getRenderCacheBackend() method, deprecated it, and created a CR.

Thanks for the detailed information you provided!

Let me know if I've forgotten something.

Yes, yes, I totally agree! I'm very concerned about naming, I always take my time to make sure I choose the best possible name. It's just that this time, I'm not entirely sure because of the questions I mentioned :)

Thanks for your suggestion, @kristiaanvandeneynde!

After reviewing it more thoroughly, I'm not entirely sure about the need for "Variation" in the method name. Perhaps "Variation" is an implementation detail, there isn’t another method that directly returns a CacheBackendInterface, and more importantly, the method is type-hinted.

However, I've made the change for others to review. 🙂

I've reordered the steps in ComponentPluginManager::getDefinitions() to avoid discovering the definitions twice and unnecessarily caching them.

Additionally, I've added a kernel test to cover this functionality.

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

Thanks for your input, @catch. Sometimes the boundaries are unclear, and I find it valuable to hear multiple perspectives and learn from them for future situations.

That's normal @drupgirl, it has to apply to the last major development branch (11.x in this case)

You can make a backport and publish it here as a patch, for others.

Traits can be a bit tricky, as they were introduced to solve the multiple inheritance problem.

I found this article quite interesting when I read it some time ago: PHP Traits: If They Weren’t Evil, They’d Be Funny.

If we consider traits as an "official" tool for developers to use across multiple tests, then you're probably right about the need for a CR. However, in this case, the trait is simply a way to reuse code without overcomplicating things.

Would it have been better to implement this with composition using a service or something similar? Or is a trait a bigger architectural change that warrants a CR to properly inform developers about this new tool?

These are still open questions for me, given my yet limited experience with the core codebase. 🙂

Thanks for your review @smustgrave!

Using a trait wasn’t the initial decision, but after @catch’s suggestion to move Functional/ShortcutCacheTagsTest::testEntityCreation() to a kernel test, it was no longer possible to reuse the functions through inheritance. As a result, they were moved into a trait.

I've updated the proposed solution in the IS to reflect this.

I don't think a CR would be necessary since EntityCacheTagsTestBase, which originally contained these methods, now uses the trait, meaning any subclass will automatically inherit them. Additionally, since these functions were not static, a static call like EntityCacheTagsTestBase::getRenderCacheBackend() would never have existed anywhere.

I've replied to your comments.

I hadn't realized that! Good idea! 🙂

I've implemented it and also extracted some common functionality into a trait.

Addressed the case where update.settings.notification.emails contained an email as a string, besides the case of an empty string.

Also updated the proposed resolution in the IS.

I want to clarify that this issue is different from 🐛 Cron tries to send update notification email while no email is set Needs work . This one addresses an error that occurs when update.settings.notification.emails is a string, which violates the config schema. This can happen due to legacy configurations present on some sites. The other issue, however, deals with update.settings.notification.emails being an array but containing only empty string entries.

Regarding this issue, I've replaced isset() with is_array() because it kept failing, and I've also added a regression test—my first one! 😊

vidorado → changed the visibility of the branch 11.x to hidden.

vidorado → changed the visibility of the branch 3353778-typeerror-implode-argument to hidden.

I've managed to fix the test, but it's not a regression test—it only verifies the specific use case reported by the original poster. However, it should trigger the error, yet I haven't been able to reproduce it by undoing the fix and running the test.

Could it be that this is no longer an issue?

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

After many attempts, I still haven't been able to solve it. 😞

I've modified the test to be a subclass of UpdatePathTestBase, but I can't update the subject field's max length. It keeps telling me: "The Subject field needs to be updated."

There's something I don't fully understand about schema and field definitions.

I've read this article → and checked out this repository, but without success.

In several places, I've seen suggestions to export the data, uninstall the field, reinstall it, and then restore the data—but that approach seems strange to me. What would make sense to me is altering the database column and updating the field definition, and the existing data should remain unchanged, since it's not a dramatic change.

Maybe someone with more expertise could help out?

@smustgrave, I've applied your two suggestions and left a reply to your third MR comment of what I think @rlmumford pretended to do.

Thanks!

@nicxvan, GitLab may have misleaded you (and me too, for a moment). In the MR overview it doesn't say that the code was changed, but it indeed was! :)

Thanks @claw, I supposed wrong then!

So, per #12, I would either

1. Close this as "Works as designed".
2. Update the documentation in settings.php and FileSystemForm, and make the test that covers the fix.

I wouldn't be confident to say that the documentation is wrong, although some people are using root directories successfully (apart from the bug). So I would need some help on deciding :)

Thanks for pointing it out @tr.

I've seen that, at least for MySQL, the utf8mb4 character set is the default for new tables, so every table will have that, although the database could have been configured with another default character set.

protected function createTableSql($name, $table) {
    $info = $this->connection->getConnectionOptions();

    // Provide defaults if needed.
    $table += [
      'mysql_engine' => 'InnoDB',
      'mysql_character_set' => 'utf8mb4',
    ];

For PostgreSQL there is no possibility of setting a different character set for each table, and all of them inherit the character set of their database, but UTF8 is the standard nowadays. The same for SQLite. I haven't searched more in-detail.

@Jandor64, I'm not sure if this could be considered a bug, since the documentation states this:

This directory must be relative to the Drupal installation directory and be accessible over the web.

/**
 * Public file path:
 *
 * A local file system path where public files will be stored. This directory
 * must exist and be writable by Drupal. This directory must be relative to
 * the Drupal installation directory and be accessible over the web.
 */
# $settings['file_public_path'] = 'sites/default/files';

But, since in #6 was said this was happening also in Pantheon (in which I doubt they would be using a root directory), I supposed that it would be happening also for non root directories, but I haven't been able to reproduce it.

I've created a test directory under my public files directory, which was the default sites/default/files, and I' ve made these tests, which are all correct:

$ drush php:eval 'var_dump(is_dir("public://test"));'
bool(true)
$ drush php:eval 'var_dump(is_dir("public://x/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://x/x/test"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x/"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/x"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://test/"));'
bool(true)

Thanks for the review, @smustgrave! I've applied your suggestions.

@nicxvan: I haven't performed the test suggested in the IS, but I believe it refers to 100 million. In my opinion, that’s an extremely unlikely case. In most scenarios, there would be only a few bulk deletions, and the gain would be minimal, but I still think it’s worth doing things the right way when the fix is so simple.

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

smustgrave → credited vidorado → .

Oh, my bad—I mixed things up again...

Now it’s clear to me: HTTP 1.0 proxies must always be skipped using a past Expires header. So, there’s nothing to fix. Thanks for rephrasing the explanation—it helped me finally grasp the full picture.

Since the third point is covered by 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed , there's nothing else to address. I agree on closing this as "Works as designed" and leaving it as is. 🙂

Thanks @znerol for a detailed response. It makes me feel better after have worked in vain, due to misunderstanding the Page Cache role in the whole picture.

I think that the only change this issue fixes, then, is setting an Expires header which honors system_performance.cache.page.max_age, which would be possibly beneficial for HTTP 1.0 proxies down the line of a response.

Before:

 protected function setResponseCacheable(Response $response, Request $request) {
    // HTTP/1.0 proxies do not support the Vary header, so prevent any caching
    // by sending an Expires date in the past. HTTP/1.1 clients ignore the
    // Expires header if a Cache-Control: max-age directive is specified (see
    // RFC 2616, section 14.9.3).
    if (!$response->headers->has('Expires')) {
      $this->setExpiresNoCache($response);
    }

    $max_age = $this->config->get('cache.page.max_age');
    $response->headers->set('Cache-Control', 'public, max-age=' . $max_age);

After:

 protected function setResponseCacheable(Response $response, Request $request) {
    $max_age = $this->config->get('cache.page.max_age');
    $response->headers->set('Cache-Control', 'public, max-age=' . $max_age);

    // If the response does not already have an Expires header, set one.
    if (is_int($max_age) && !$response->headers->has('Expires')) {
      $response->setExpires((new \DateTime())->setTimestamp($this->time->getRequestTime() + $max_age));
    }

Which I believe is not in the scope of the original issue and could fit better as an additional task to 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed .

But, as you said, a problem remains unsolved:

PageCache determines which responses to cache by inspecting the Expires header. (This should be updated to use the Cache-Control header instead, but that's out of scope here.)

So, what's next? I'm not sure at all...

1. Bring the discussion of max_age config parameter honoring to 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed
2. Close this issue as "Works as designed" or leave only the fix that fixes the Expires header and go on to Fixed? I'm not sure if it's beneficial for HTTP 1.0 proxies or if they really exist yet nowadays.
3. Create another issue to fix the mentioned remaining problem with PageCache inspectiong Expires header instead of Cache-Control, so this one don't get "noisy"?

Am I right? :)

I'm not concern because State is a cache collector and it has to get all the values before a set... - see the call to $this->lazyLoadCache(); in both get and set in the parent cache collector class.

I see. State data is cached in-memory in the CacheCollector::$storage variable, so is cheap to retrieve the value with a get() :)

Since all threads and suggestions are resolved, the test are passing (I've launched another pipeline and it passed, the failing tests were the common ones that randomly fail and you must re-run), I believe we can switch to RTBC.

Thanks to all for your work! I've learned a lot from this ^^

That's elegant! 🙂 Thanks for helping out!

Sorry, I wasn’t getting it at first when I read #67, but after view it in code, I got it! :)

I've also learned from it that a complete class constructor parameter promotion can be in the scope of an issue that modifies the class (which I think is cool), and also learned how to do a proper "autowire service promotion".

Overall, I think the code has turned out to be a very lean and clean change.

It works fine, either with Drush or the UI.

Only a question remains:

Should we be concerned about the performance impact of $this->registerKeySetDuringRequest($key, $value, parent::get($key)); being executed for each key set in State? (for the parent::get($key), which will have a cost).

Thanks @alexpott,

I hadn't realized that firing another event during the termination event could be problematic. Thanks for pointing it out.

However, the termination event we were subscribing to was intended to collect all the state keys set during the request, since at that stage, it's very unlikely that any more keys would be set. In contrast, the StateKeySetEvent I suggested would be fired each time a state key is set, rather than at termination.

With this approach, a MaintenanceModeEventSubscriber could listen for system.maintenance_mode being set to 1 and log a message if the original value was 0. This might generate a log entry each time maintenance mode is set during a request, but it's highly unlikely that it would be set more than once.

Would this event subscriber align with the "maintenance mode logger" you suggested? Were you referring to a polling strategy rather than a publisher-subscriber approach?

By the way, now that this fix ensures the system.performance.cache.page.max_age config value is properly honored by PageCache, I'm not sure how it will overlap with 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed , but it's certainly related.

I've finally fixed the caching behavior and all tests.

Things I'm concerned about:

• I had to remove the weak (W/) prefix from the ETag header, as it was sometimes being added, causing multiple EntityResourceTestBase tests to fail. I was able to reproduce the issue, but I'm not entirely sure if it's due to an Nginx configuration or something else.
• Now that system.performance.cache.page.max_age is being properly honored, I had to set a max_age value in several tests expecting responses to be cacheable. I'm not sure if this is the correct approach or if we should configure it globally in BrowserTestBase.
• I've assumed that having two Vary headers in the response is correct, but I haven't determined why they are being set separately. An additional Accept-Encoding value is added in a second Vary header instead of being merged into the existing Vary header containing Cookie and Origin values. To account for this, I modified CorsIntegrationTest to check for "Origin" across all possible Vary headers.

After digging into the code, I realized I made a mistake. To prevent multiple tests from failing, I mistakenly kept the very piece of code that was wrong in the first place:

  $expire = ($date > $request_time) ? $date : Cache::PERMANENT;

I also confirmed that this issue is only related to the Expires header and not to PageCache’s behavior of ignoring max-age. For that second issue, please refer to 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed @johnv.

I have another idea, although it's not exactly in linew with @alexpott suggested:

What if we fire a dedicated event for state changes, such as StateKeySetEvent, to which a MaintenanceModeEventSubscriber could listen and log a more specific message?

We could dispatch the event before the key is updated, allowing subscribers to retrieve the original value directly from the state instead of passing it within the event, only in case they need it. This would introduce a performance impact, but only for subscribers that actually need to access the original value. Given that there would only be a few subscribers, the impact wouldn’t be as significant as retrieving the original value for every key set in a request.

What do you think?

I'm not feeling as confident with naming as I usually do while writing this code… maybe it's just late, and I'm not thinking clearly. 🙂

• I'm unsure about the StateKeysSetLoggerSubscriber subscriber name.
• I'm also not completely sure about the service name state.keys_set_logger.
• I feel uneasy about not being able to provide a more specific log message when maintenance mode is set, rather than just logging "The state key system.maintenance_mode has been changed from 0 to 1."

Nonetheless, I believe this approach addresses #60 and the concerns discussed earlier.

Any feedback would be greatly appreciated!

Sounds good. I'll work into this :)

Umm, this is getting interesting. Some thoughts:

1. If system.maintenance_mode is set multiple times within the same request or CLI operation, the last change could be from 1 to 1, which wouldn’t be useful. However, that’s very unlikely to happen, though.
2. If you believe that the final value is what matters most, then storing the previous value isn’t relevant.
3. On the other hand, if you consider the previous value important, I start to worry about an edge case like the one mentioned in point 1.

@smustgrave I've replied to your comments.

Thanks for the review!

Ow, yes, you're right. I was thinking about Drupal 12, but the CR is for the Drupal 11.x minor version where this issue will be merged.

Sorry, I'm yet a CR rookie! xD

I tried to clarify them a bit :)

Thanks!

Thanks! I've replied to your comment in the MR.

Addressed! :)

@jonathan_hunt, if you add the "public" protocol in a link field configuration (after applying the changes in this issue fork: https://git.drupalcode.org/issue/drupal-2031149) you will be able to add links like public://whatever/you/want/file.pdf, but those URIs will not be converted to an URL. You will have to process them in a custom hook.

Maybe the module Linkit ( https://www.drupal.org/project/linkit → ) would address what you want? Although a relative path would be stored in the field, instead of a public:// style URI, the public directory is not likely to change, so it would be enough for you, depending on the case.

vidorado → changed the visibility of the branch 11.x to hidden.

This was a tough one, but I believe I've managed to sort things out.

Some unexpected behaviors were occurring, such as the FinishResponseSubscriber setting an Expires header, which made the response non-cacheable—inside the setResponseCacheable() method, which should theoretically do the opposite.

Additionally, some functional tests in PageCacheTest were incorrectly expecting the response to be non-cacheable when it actually should have been. As a result, they were effectively masking the issue with cacheability headers.

A more in-deep review is necessary, as this is a complex scenario with multiple edge cases involved, and a thorough understanding of the HTTP cacheability headers specification is required to do so.

Also, I haven't been able to mark the MR as "Ready" I'm not sure if someone with higher privileges could do it, so I don't have to create a new one.

vidorado → changed the visibility of the branch 11.x to hidden.

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

@joseph.olstad Perhaps what you expect is only valid for your specific use case?

When creating a node in French, the preview displays the original, not-yet-saved French translation of the node, and the tag is shown in the entity's language. Isn't that the expected behavior?

I believe we shouldn't suppress errors caused by a contrib or custom FieldFormatter. The return type of FieldFormatterInterface::viewElements() is explicitly defined as array, so no formatter should return NULL.

I would close this as "Works as designed."

Trying to get feedback

vidorado → created an issue.

I believe this is a significant change that could cause WSODs after an update. Instead of throwing an exception, we should trigger a deprecation error first.

I've also fixed some failing tests caused by a core route that defined a route requirement for a menu_link_content entity—which has bundles—without specifying a bundle.

Additionally, a change record has been issued. I will create a follow-up issue to replace the deprecation error with an \InvalidArgumentException.

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

The last MR commit was silencing the filesize test, which was failing after the fix, so I've restored it. Additionally, I've tested also that the file size is only validated over new files.

@kim.pepper, thanks for pointing the deprecation out. I've seen that it was already addressed in the existing MR commit.

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

First of all, congratulations to @seanb! The media library code was not trivial, and their fix was both well-executed and clean.

After manually testing, I noticed that the langcode was not being properly set in the translation add form. To fix this, I modified the code to retrieve the langcode from the form_state storage, which turned out to contain the target translation’s langcode.

Since the documentation states that "no specific support is provided for it in the Form API" regarding the FormState::storage variable, I added a fallback to the original fix $entity's language, which is for sure always available:

  $form_state->get('langcode') ?? $entity->language()->getId(),

Additionally, I’ve included an FJ test. :)

I've added test coverage.

As a bonus, it turns out that we've fixed, at least partially, a bug stated in 🐛 Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error Closed: duplicate , so I have had to change a line in BlockUiTest too.

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

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

I've added a kernel test to ensure that a single delete query is executed against both the data table and the revision data table.

I'm not sure if directly testing the protected deleteFromDedicatedTables() method is the best approach, for the sake of simplicity, or if I should have mocked even more components to test it only through the public interface of SqlContentEntityStorage.

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

Changing version to latest 10.x as I see in the IS that this is obsolete for 11.x