🇩🇪Germany @tstoeckler

tstoeckler → created an issue.

Thanks for the kind words, that's really nice to hear. No worries, that's how it goes, as you saw it also took me quite a while to get back to this.

Added a kernel test that tests the scope granularity permission handling and a functional test (which has to be a Javascript test because of the Ajax stuff) for the scope granularity form. Let me know if there's anything else you would like to see tested in particular.

Speaking of excellent test coverage... I had forgotten to run the simple_oauth_static_scope tests locally. That actually revealed a couple of nice bugs, as well, as I had only tested it with dynamic scopes locally 👍

Also fixed PHPCS and PHPStan violations. As I had opted to change the configuration structure for static scopes, as well, and add a deprecation for the now-legacy format, that required me to write a draft change notice so I did that for now. All of this is of course up to debate, worst case we can delete the change notice without publishing it I guess.

As far as I can tell the "cspell" and the "phpunit (next minor)" failures are not related to this MR, so ignoring those for now.

...and now actually did the self-review as promised.

Alrighty, took a bit longer than I hoped to get back to this, but here we go. I went ahead and implemented this as described in the issue summary.

First off, I want to shout out the great test coverage of this module. I had tested that everything works locally already, but the tests caught a bunch of errors and edge cases that were broken. Really nice work!

The MR is "finished" in the sense that it works, tests pass and I converted all code to the new structure. I did not yet add a specific test of the new plugin functionality. I.e. my thinking is to create a test module with a test granularity plugin and then test that Oauth2ScopeProvider::scopeHasPermission() returns the correct things. But I thought it makes sense to start discussing the actual implementation first at this point. I tried to make the code feel "at home" in the current codebase as much as possible, but there are bunch of things that are arguable, I will do a self-review on Gitlab to maybe kick off some discussion points. But of course input on any parts of the MR is much appreciated.

Awesome, thanks for the quick turnaround!

Now that a bunch of the bigger "per-hook" issues landed, I looked into tackling this on a "per-module(ish)" basis and started with the biggest offender: 📌 Add return types to update_test_* hooks Active

Hope that's OK and would love a review.

tstoeckler → created an issue.

Thanks for the review, fixed all the things now (hopefully).

There was an issue in #32 in how getAsArray() was used, that is fixed now and, thus, the tests are green again. Good to go from my point of view.

tstoeckler → created an issue.

If so we would need a different approach, maybe we deprecate the old

I don't understand, that's exactly what the patch does.

Added a draft change notice and converted to merge request.

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

Re #4: Thanks for the review! Added a draft change notice and the in-code deprecation. The merge request already adds a bunch of assertions to EntityAddUITest so what test coverage are you looking for exactly?

Re #5: I understand the use-case totally, but given that it's already hard enough to find support for this very targeted issue I don't believe broadening the scope is wise at this point. Feel free to post a separate MR (here or in another issue) for your use-case but I will continue with the current approach for now, sorry.

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.