🇩🇪Germany @tstoeckler

Thanks for the thorough checking @quietone and the detailed instructions @mstrelan. I fixed the ones you mentioned, hope I didn't miss anything.

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

tstoeckler → created an issue.

Thanks for the patch! Can confirm that this works. I think it's quite neat to not have to specify the key and secret, in particular, in settings.php directly, but just reference the profile and have the credentials in ~/.aws/credentials.

Still want to get back to this just wanted to note that this needs to be adjusted to add a provider to the menu link. Otherwise MenuLinkDefaultForm blows up, unfortunately. The value should be the entity type provider.

Since I just encountered this beauty of a bug once again (yes, don't ask...) adding another suggestion to fix this here that was proposed by a certain ghost of drupal past on Mastodon during this year's Drupal Dev Days so that I don't have to dig that up again. Slightly altering/rephrasing the details, but I cannot take credit for the idea itself. Will refer to this as "Suggestion 4" for now:

• Rename getAllBundleInfo() to doGetAllBundleInfo() (or similar).
• Remove the fetching of the labels from doGetAllBundleInfo() and, thus, from the cache entirely. Instead just load the label dynamically in getBundleInfo(). (We could also consider adding a new getBundleNames or something if you just need the keys and not the labels, but that's a minor detail (and possibly a follow-up).)
• Re-instate getAllBundleInfo() to just call getBundleInfo() for all entity types. It will be slower than before but it's not a very common use-case to need bundle information for all entity types. It could also easily be deprecated and have the few cases where it's used in core just move to calling getBundleInfo() for the entity types that it needs.

I personally think this is the best suggestion so far. Would love to hear thoughts on this!

Fixed

Thanks for this, this is very helpful!

I have run into this issue lots of times and have come into the habit of copy-pasting the following stanza to various services in various projects for exactly this purpose:

  protected static function reclaimMemory(): void {
    $memory_cache = \Drupal::service('entity.memory_cache');
    assert($memory_cache instanceof MemoryCacheInterface);
    $memory_cache->deleteAll();
    gc_collect_cycles();
  }

I never figured out how to properly solve this issue, though, great work on that! Not sure how to write a test for this, as the concrete memory usage at any given point is always a bit unpredictable, but maybe someone will have an idea. Definitely bookmarking this and will give this a spin when I get a chance.

Fixed that, thanks!

Exactly 👍

There is no practical difference, it's just that the (non-enforced) dependency gets added when importing the configuration anyway, so this makes the shipped file match the state that will be imported.

Read through chunks of this issue again and as far as I can tell two separate things are being discussed here and while both of them are related to the cache max-age and its (lack of) bubbling, only one of them seems to be the cause for the seeming "intractability" of this issue:

1. Things having an explicit nonzero, finite max-age
2. Things setting their max-age to 0 to declare themselves as uncacheable

While I would absolutely applaud also getting the second use-case somehow properly integrated with page cache it seems getting the first use-case to work is technically just as simple but does not have any of the repercussions in terms of page cache integration that the second one does.

So I would like to propose to solve that issue first, i.e. to bubble nonzero max-ages into the page cache and explicitly leave the "max-age = 0"-case as a todo for the future.

The only place in core we ever set a nonzero, finite max-age, as far as I know, is the time-based caching plugin for views, so the only affect this would have in core is those views actually being properly expired by page cache. But the benefit would be that any contrib or custom modules that either (knowingly or unknowingly) do not work with Page Cache or somehow have to employ hacks to set the response expiry manually will now work with Page Cache automatically (and without any hacks).

Would love to hear if people agree with the splitting up of this problem and if so, whether or not the first (nonzero) part should happen in this issue and the zero part be handled in a follow-up or if we should keep the zero part here and split out the nonzero part.

Pretty sure this was fixed in 📌 UniqueFieldValueValidator works only with single value fields Fixed / https://git.drupalcode.org/project/drupal/-/commit/3981c8aa1327da7d29cdd... as the check now is for $entity->isNew().

Please re-open if this is not the case.

Looked into this, and, yes, there this does break things when working with translated content. EntityReferenceRevisionsCompositeTranslationTest::testCompositePendingRevisionTranslation() creates an English node, then creates a pending revision for that, then a German translation and then a pending revision for the German translation. The node_field_revision table then (after line 232) looks like this:

With this patch it looks like this at the same point in the test:

I didn't investigate any further, because I now want to go back to see exactly the use-case why I needed the patch in the first place in the light of this finding. But wanted to share this in case this helps anyone else push this along or provide further insight.

Did some very light Git archeology but wasn't able to find anything interesting. As far as I can tell this was duplicate already 9 years ago when it was introduced.

Also included the removal of a missing & unused use statement, but as a separate commit in case that's considered out of scope.

tstoeckler → created an issue.

Re #16: As I noted in #12 these methods are helpful for contrib/custom code, so I don't think your argument is very convincing

Right, as noted in #3 the plan is to deprecate the existing method.

Re marking the methods as protected: I actually disagree with the notion of that @todo, i.e. with what is being pursued in 📌 Convert DefaultTableMapping::setFieldNames() and ::setExtraColumns() to protected methods Active . In my opinion we should just remove the @todo and leave the method(/s) public. But since the existing method has that todo it seemed sneaky to just remove it wholesale, just because I disagree with it. On the other hand, it also doesn't seem fair to introduce it as protected right from the get-go if there is no consensus on 📌 Convert DefaultTableMapping::setFieldNames() and ::setExtraColumns() to protected methods Active yet.

tstoeckler → created an issue.

Nice catch, thanks! 👍

That documentation is actually already incorrect prior to this issue, so opened 📌 Update EntityTypeInterface::getKeys() docs for string IDs Needs review about that. We are not changing anything about being able to use strings in the first place, this is just about using UUIDs, in particular, and to be more precise to be able to use the same field as UUID key and ID key.

Created a quick MR.

Since this was directly in scope of the lines being touched, I opted to touch up two other things:
- Removed the note about the ID key being required. It is not, it is only required if you actually want to save the entity. That of course means in 99% of cases you do want to specify an ID key, but with contact messages in core we actually have an example of not having an ID, so it doesn't make much sense to claim it's required.
- Removed the usage of the term "Field API". It is a remnant of D7 times. In current times it is actually in correct because getKeys() is used for config entities as well and they have nothing to do with Field API in its current form. The more correct/current term would be "Entity API", but instead of replacing that I just removed the explicit mentioning of that as that would have felt somewhat out of place there.

I also noted that we use the term "property" there, to describe the values of the keys array. That's also somewhat incorrect, as for content entities, those are actually field names and properties are something different, but for config entities we do tend to use the term properties (for example ConfigEntityTypeInterface::getPropertiesToExport(), so I think it might be more clear to consistently use "field or property" instead of just "property". That would need to be changed a bunch of times, though, in that documentation paragraphs, so I anticipate that being flagged as too broad in scope for this issue.

tstoeckler → created an issue.

Rebased the MR for 🐛 EntityViewsData Column information not available for computed entity reference fields Fixed

Don't want to directly add it to the issue summary, but I immediately thought of #2350509: Implement auto-route generation for all core entities and convert all of the core entities. → when reading this...

Fixed now, thanks again for raising this.

quietone → credited tstoeckler → .

tstoeckler → created an issue.

CI will be fixed over in 📌 \Drupal::config('system.date')->get('country.default') now has validation and can now be null Needs review and I don't have the time to investigate the rector issue so closing this.

Thanks for opening this.

Will fix this and part of that fix the CI and bring it up to date at bit.

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

Thanks, merged the MR!

Thanks, committed the patch as is!

Closing this for now as there is nothing actionable here, as far as I can tell. Feel free to re-open if there is in fact a bug or problem that we can fix.

Closing this for now. Feel free to re-open or open a new issue if there is a bug or problem that I am not aware of.

valthebald → credited tstoeckler → .

Awesome, that's great to hear, thanks for the usability review to all those involved. Will try to get an updated MR going here soon, but can't promise anything...

Yup, back to RTBC then. Just pushed the patch to an MR, so the RTBC from #14 still applies.

Re #79: That's the entire point of this issue: For those examples that you mentioned there is no point in distinguishing between the off-value ("Do not promote this to the front page" / "I do not want a child meal") and no value ("Not known whether to promote this to the front page or not" / "Not sure if I want a child meal"). And in terms of Field API "required" means "there must be a value", not necessarily "the on-value / yes-value / ... must be chosen". And if you do in fact want to allow making a distinction between the off-value and no value then the checkbox is not a suitable widget for that, because it inherently cannot make that distinction.

That said, I absolutely welcome any suggestions on improving the in-code documentation, as apparently it's not as clear as it could be.

Well the CI fails, but not on the added test method, so I guess back to RTBC.

Wanted to show the difference by showing the before/after output of the "Validatable Config" job, but that one runs the Drupal install on the previous codebase, so we cannot in fact add that option there directly. So removing that change now, but added a note to the issue summary about the quickstart test change.

For reference, here is an example of the output in current HEAD for the Validatable Config job:
https://git.drupalcode.org/project/drupal/-/jobs/2038037#L160

I don't really have an opinion on which approach is better, but this looks good to me as well. Updated the change notice for the new approach, so as far as I can tell this is good to go again.

I can't access the old DrupalCI test results anymore and the Gitlab CI testing is broken currently for 8.x-1.x anyway, so not really sure that #9 is still relevant.

I hit this as well and the patch solved the issue perfectly, thanks a lot @travis-bradbury. Nice catch with the translation issue, as well! 👍

It also comes with test coverage which is also great.

RTBC from my point of view.

Great, thanks! 👍

Awesome, thanks. You dropped my commits from the MR, though, I'm guessing that was accidental? 🤔

...ahh maybe because the MR is targeting 10.1.x. @bradjones1 do you have access to change the target branch? Or should we create a new MR? Not sure...

Opened a merge request with the change. This will need to be changed to deprecate setFieldNames() before it can go in, but I would like some discussion/agreement that this makes sense first, before going ahead with that. Thus, marking "Needs review".

Not sure why the testbot doesn't run for the updated commits, but I opened 📌 Replace DefaultTableMapping::setFieldNames() with ::add*() and ::remove*() Needs work in the meantime.

tstoeckler → created an issue.

Just stumbled upon this. Currently there is not really an easy way to modify the default table mapping, in my opinion. Unless you actually want something radically different providing a different (sub-)class other than DefaultTableMapping, while possible, is not actually very helpful, because you may need to copy the entire class just to change a few lines. Having these methods public, currently allows avoiding that and actually making small alterations with just a small amount of code. See 🐛 Cannot use UUID as entity ID Needs work for something that is currently fixable fairly cleanly from custom code which would become impossible, not in principle, but in reality, if these methods were made protected.

Since this also does not bring any maintainability gain as far as I can tell, I am against this until we have a different more bespoke API for actually customizing the entity table layout.

After talking to @rnsrk abou this in Burgas, spent some more time on this.

Fixed the issue mentioned in #9. Also added some inline comments about the changes and finally added some test coverage. I pushed those to individual commits in case there are any objections to any of those.

Note that I thought a bit about how to avoid all this array madness (i.e. array_values(array_unique(array_filter(...))) and friends) and I think the solution would be to make DefaultTableMapping provide an addFields() method, which would internally check for duplicate fields. Will open a follow-up about that, so we can avoid that discussion here (even though it does make the situation (inherently) slightly worse).

Not exactly why this was tagged "Needs change record" or what the change record should be. I mean we could literally write a change notice with "Content entities may now use the same field for the ID and UUID keys" if people think that's useful. (I don't, in particular, but it also doesn't hurt, so I wouldn't mind it.)

So as far as I can tell the following is true:

1. We have an email.validator service that uses egulias' one but does not perform strict validation (it uses RFCValidation())
2. Email form elements use this service
3. Email fields do not use the core service but the Symfony constraint which also uses egulias' validator but does perform strict validation (it uses NoRFCWarningsValidation())

The discrepancy between form elements and fields is already very unfortunate in my opinion in it's own right. It get's worse, though, because of the fact that on site install the site email (form element) is used as the user 1 email (field). Thus, you can currently install Drupal using foo@localhost as a site (+ user 1) email, but that leaves user 1 in an invalid state. In particular, you cannot submit the user 1 profile form (without changing the email to something else).

As far as I can tell, the decision to prefer strict validation in core has already been made by #3307736: EmailValidator defaults to 'loose' mode which is deprecated in Symfony 6.2 → so I think the solution would be to just make the email.validator service use strict validation, as well, but holding off on the title and issue summary and the re-classifying as a bug until some confirmation on the proposed the resolution.

tstoeckler → created an issue.

tstoeckler → created an issue.

tstoeckler → created an issue.

Awesome thanks!

...and here's a patch that implements 4. Not doing an MR as per #2 this would not be my preferred way of going forward but it is the least invasive workaround for people hitting this on their sites.

Since I have it laying around locally, maybe it helps someone: Here's a patch that adds an "oEmbed request log" by means of simply writing a log entry each time the oEmbed subsystem performs an HTTP request.

I pursued solution 4 initially for a bit locally, but having written the issue summary now, I think 3. is the way to go. Conceptually media metadata is allowed to be "expensive" and that's exactly why we have the concept of mapping it to local fields if it needs to be accessed regularly.

tstoeckler → created an issue.

Just hit this as well. Noticed that \Drupal\Tests\BrowserHtmlDebugTrait::htmlOutput() needs to be updated, as well.

tstoeckler → created an issue.

Really wavering back and forth on this one a bit...

Tried what @bircher mentioned in the core issue in #5: To add the config overlay config transformer via settings.php. That's not super intuitive, though you need this whole things in your services.yml:

services:
  # See config_overlay.services.yml
  config_overlay.extension_storage_factory:
    class: Drupal\config_overlay\Config\ExtensionStorageFactory
    arguments: ['@extension.list.profile']
  config_overlay.config_subscriber.transform_early:
    class: Drupal\config_overlay\EventSubscriber\ConfigTransformEarlySubscriber
    arguments:
      - '@extension.list.profile'
      - '%install_profile%'
      - '@config_overlay.extension_storage_factory'
      - '@config.storage'
    tags:
      - { name: event_subscriber }

And then, because Config Overlay is not in the autoloader by default, the following in your project's composer.json:

    "autoload": {
        "classmap": [
            "web/modules/contrib/config_overlay/src/Config/ExtensionOptionalStorage.php",
            "web/modules/contrib/config_overlay/src/Config/ExtensionStorageFactory.php",
            "web/modules/contrib/config_overlay/src/Config/ReadOnlyUnionStorage.php",
            "web/modules/contrib/config_overlay/src/EventSubscriber/ConfigTransformEarlySubscriber.php",
            "web/modules/contrib/config_overlay/src/EventSubscriber/ConfigTransformSubscriberBase.php"
        ]
    },

Not sure if there's a way we can tidy that up a bit, but not sure I want to actually recommend people doing so, as it seems like a lot of work...

Not sure about #5, because as far as I know all modules will still be installed first even in an install-from-config. But didn't verify that.

However, I agree it does kind of make sense to have a dependency on user module, even if there is no concrete functional bug without it. I was about to write we should fix the core issue I opened to get that dependency for free, but after looking at that one a bit I realized that in core all config dependencies, including those on entity type providers, are handled by the respective action plugins and that the type config property is actually unused entirely. So I think what we should do here is have UpdateUsernameAction add the config dependency on user module.

Added a commit to the merge request which does so by extending EntityActionBase. (For full disclosure, I was feeling bold and used the Web IDE for this, but since there is test coverage for the action (👏👏👏!!!), I hope that's all right.)

Was in the process of creating a merge request for this, but now repurposing this instead (without a merge reuqest for now). I had thought that the type of action configuration entities is in fact configurable in some way. For example, that the entity:publish_action plugin ID is used regardless of the entity type and the entity type is determined by the type set in the action configuration. That's not actually the case, however. Instead, there is a plugin deriver that derives plugin definitions for all applicable entity types so that the actual plugin ID being used is, for example, entity:publish_action:media. The type is then just copied over from that plugin definition's type property. This is also reflected in how the (now contrib) UI does things.

And all those entity-type-specific, derived plugin definitions the config dependencies already end up being set up correctly, because the (derived) plugins add their entity type provider as a dependency and that then gets picked up by the action entity due to the use of a plugin collection.

So there really is no bug here unless we explicitly want to support setting an entity type ID as an action type for an action that does not extend EntityActionBase. Instead, the question is why store the type at all, if it's just a "denormalized" version of the plugin's definition's type.

The only usage I could find in core is in views_views_data() where Views decides whether or not to add a $entity_type . '_bulk_form' views field to an entity type depending on whether there are any actions for that entity type. But that check could easily be changed to check the action plugin's definition instead.

So I propose deprecating the type configuration property of actions.

Re-titling and updating the issue summary, accordingly.

Hit this again, so shameless bump after all this time, would be nice to get this in (even though the minor priority is definitely still deserved). Thanks!

Note that I opened 📌 Action config entities should have a config dependency on their (entity) type provider Active in core because I do think that in principle the dependency on user is correct because that is the entity type that the action is for. Note that that could in theory be done via an enforced config dependency, but I'm not sure if it's really worth it because user module cannot be uninstalled anyway, so it's quite a theoretical case.

tstoeckler → created an issue.

tstoeckler → created an issue.

Hmm... yeah I think all of that stuff can be deprecated in favor of pointing people to 🐛 Dispatch config transformation event during site install from configuration Active if they encounter any installation issues.

Tentatively renaming the issue...

Ha, noticed that we cannot use ModuleHandlerInterface::loadInclude() as Config Overlay is not installed at that moment (inherently), so we need to manually require the file. Updated the issue summary...

tstoeckler → created an issue.

tstoeckler → created an issue.

Wow, that's awesome, thanks a lot!

tstoeckler → created an issue.

Thanks for taking a look @alexpott!

Basically the tl:dr; of the below is: would love to hear more of the thoughts behind your comment ;-)

Really tricky issue and great sleuthing on working about what is good on. The issue summary is impressive.

Thanks! That really means a lot.

The MR feels very much like a sticking plaster. I'm not sure you'd expect different results from Entity::getTypedData() depending on when it is called.

Not sure what you mean by this. One symptom of this bug is exactly that Entity::getTypedData() may return different results depending on what happened before. In particular, in the example in the summary $term->getTypedData() would incorrectly return a typed data instance of entity:taxonomy_term and then if for some reason only the typed data cache was cleared it would then correctly return an instance of entity:taxonomy_term:tags. Adding this to the issue summary in fact, because I think it's helpful context, thanks for bringing that up! So, yeah, the MR doesn't feel great, 100% agreed, but maybe you can elaborate what you mean by this specifically.

Do you think that it is worth deprecating the recursive call into \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo() and in a future version of Drupal throwing a exception when this happens.

Again, not really sure what you mean by this, unfortunately. Will take some stabs at a guess, but maybe you can elaborate here, as well:

1. Are you suggesting to not have bundle-specific data types at all? Not sure how we would properly validate bundle-specific validation constraints if we were to do that. But that would very cleanly solve this issue by actually properly getting rid of the circular dependency. (To be clear, not necessarily implying you actually suggested this, just trying to explore all possible options here.)
2. Are suggesting to not allow fetching the typed data information when loading a (bundle) entity? In theory I guess this would be the less invasive half of the circle to break, as the use-cases described in the issue summary (while valid) seem less important than having bundle-specific data types at all (i.e. 1.) Not really sure how to achieve this, though, as this would mean calling $typedDataManager->getDefinitions() without a primed bundle cache would yield a (deprecation notice, but eventually an) exception?
3. Having written the above and looked at the issue again, I had an idea of a maybe less terrible way to solve this issue at hand, so just listing it here to make it easier to refer to down below: Avoid loading entities at all for the building the bundle information. We really only need the IDs and labels of the entities, so we could fetch those directly from the storage (either via an entity query directly or via a new dedicated method on the storage). The only problem would be entity types that override label() and do something fancy there, but maybe it's fair to deprecate doing that for bundle entities (because presumably we don't want to just wholesale deprecate overriding that method)? While throwing the deprecation notice / exception would then require something like the somewhat ugly method_exists($entityType->getClass(), 'label') it would at least be a way forward that does not depend on the internal state of the typed data manager.

Awesome, thanks for the endorsement, will try to cook something up. Yeah makes sense with the test coverage 👍

Ahh that's a neat idea, hadn't thought of that 👍 Thanks!

No worries about the issues, all good.

Closed as duplicate of 🐛 Dispatch config transformation event during site install from configuration Active

I recently opened 📌 Config storage transformers do not get applied when installing Drupal from configuration Needs work for this, because I hit this with Config Overlay, as well. I only had the change in the "revert" step there, which seems to be sufficient at least according to Config Overlay's test coverage ;-). I left out the change in the initial import because as far as I can tell, there are no modules installed at that point anyway, so I felt like there's really no benefit to it. And ExcludedModulesEventSubscriber (which is the only transformer in code) will not have any effect on the initial install either, as far as I can tell. Just curious about the reasoning there, not at all opposed. In fact, if it doesn't break anything, it does seem more "correct" to do it in both cases. I guess if we do add another config subscriber in core it's nice to have that already affect the initial import and not just the "revert". In any case, will close my issue as duplicate.

That's a fair point. 👍 That would probably be helpful for people evaluating the module.

Not sure when I will get to it, but will try to find some time.

Fair enough! Thanks for the pointer, haven't looked at Config Devel in ages, that does seem like a real useful companion module for Config Overlay. 👍 Will try it out, but seems to be exactly what I was thinking about from the look of it.

Will add a note to the project page about that.

Leaving open for that, for now, and also because I do think it would be nice to have some sort of "automatic" mode without having to specify all configuration explicitly. Especially for project-specific installation profiles, that list is pretty huge and does change somewhat regularly. But nevertheless, very useful hint and maybe such a feature would even be something to consider in Config Devel itself.

I would be willing to try to implement this, but wanted to check first, whether this is something that's considered desirable since it may serve somewhat of a niche use-case. On the other hand this does get rid of a couple if-else constructs in favor of a (what I would consider "proper") plugin system, but while I think that's a good trade-off in its own right, it's not my decision to make. So would love some thoughts on this, thanks!

tstoeckler → created an issue.

Oops, got the screenshots mixed up.

tstoeckler → created an issue.

Glad that you got it to work, nice.

Sounds like a good plan regarding the issues.