🇩🇪Germany @tstoeckler

Created an initial MR. Feedback on all parts very much welcome.

One thing of note: I had originally implemented the form as being opened in a modal, but then hit 🐛 Form validation breaks out of a dialog Active which is why I reverted that.

tstoeckler → created an issue.

tstoeckler → created an issue.

I opened 🐛 Routes are incorrect after installation from config Active for a related issue and just opened a merge request over there. Just to be sure, can you check if the patch there solves your issue? I was having a different problem, so not entirely sure.

Interesting, hadn't seen that. Yes, they are related in the sense that I am proposing to remove the check that is leading to that error. On the other hand, I could not reproduce that problem so not sure removing the check would not lead to other errors for the person experiencing that. In any case, will open a merge request to remove the check and see what the tests say. (As noted in the issue summary manual tests showed that that is sufficient to solve the issue described here.)

There is #3109887: Find a more reliable way to determine if a form element concerns a multilingual value. → also which tries to solve by making this an element property. This can be altered via hook_element_info_alter() which I think would be preferable to the setting and in my opinion is better suited for such a "niche" feature than a container parameter. Technically this issue precedes that one, so not sure if that one should be marked as a duplicate or if we should just continue there instead. Either way needs to be converted to a merge request and needs some tests.

Discussed this with @harlor at DrupalDevDays, just noting some thoughts.

Since we already use the class resolver to instantiate the form class, it should work fine to use service IDs instead of the actual class names (or otherwise just have the class name be the service ID) and the decorate the respective services. So by simply making forms services and referencing them by their ID we could already reap the benefits of being able to decorate them.

On the other hand, years ago there were discussions already about converting forms to plugins in the context of Config Translation. Config Translation basically needs to now a title and a path for a form and currently dynamically collects that information during route building, but it would be nicer (and less brittle) to have that information statically. That would then also allow altering forms very easily.

Either way, really like the idea 👍️

Nice work finding #3011744: File/image widget validation is inconsistent based on cardinality/visibility → . I think it makes sense to keep this open separately as a distinct bugfix (which is also not caused by #2715859: ImageWidget::validateRequiredFields() produces an error message if an image field is hidden with hook_entity_field_access() → which #3011744: File/image widget validation is inconsistent based on cardinality/visibility → is a follow-up of). Then that issue can take care of the issue of hidden widgets and the cardinality issue can be dealt with here.

Re #23, I think the existing test coverage is fine in image module, but I do agree that this needs some explicit test coverage in file module. I looked around a bit and I think it would make sense to add some assertions to \Drupal\Tests\file\FunctionalJavascript\FileFieldWidgetTest::testMultiValuedWidget. There we are already creating two fields with cardinality 3, so we would only need to submit the form and assert that the respective error messages are displayed before going on with the rest of the test. Tagging accordingly.

Otherwise looks great, nice find and nice fix! 👍️

Hmm... strange, don't really understand why the Nightwatch tests would fail, but I guess would be good to get a green pipeline before this goes in.

Wow, what awesome turnaround, thanks a lot. Looked through the changes and it looks great to me. I did spend some more time looking at the other usages of getMainPropertyName(), but I'm not yet convinced there's actually anything actionable there, so not opening a follow-up issue for that. So there's really nothing left to do here from my point of view.

In starting to write the change notice and looking at some of the code I found that - in addition to the current assumption of the main property that is discussed here - JSON:API currently also already assumes a computed entity property that is specifically called 'entity'. See \Drupal\jsonapi\IncludeResolver::resolveIncludeTree(). So what I said above is not true, there is no change that this property is required - because it already is. I think that is further evidence that the proposed approach is the correct and this is just consolidating different assumptions in different places to one assumption.

Did not intend to unassign, apologies.

Yes, thanks for managing the MRs @pwolanin that makes it more manageable. Yes, I agree with the proposed resolution. The only way this would be a breaking change would be if a custom entity reference implementation either did not provide a computed entity property at all - which would (among lots of other things!) break the entity query joining across the entity reference while having absolutely no benefit - or did provide one but did not manage the sync from the entity property to its target ID property - which just seems like a straight up bug. So I don't think this should be a reason to hold this up, in particular since this is an actual contrib blocker. I can't really imagine that there are actually people impacted by this, but will write a change notice for this just in case anyway.

And will do a proper code review afterwards.

Coolio, will tag a new alpha now.

tstoeckler → created an issue.

Had to touch up one minor thing, but otherwise looks good, thanks!

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

As far as I can tell this was worked around in ✨ Use modal in add new field flow Active (for that particular case) but have not delved into it in depth if that is in fact the case and, if so, if that be generalized in some way.

Not opening an MR yet before getting some confirmation on the direction here (and also because the workaround is so trivial that I don't need a patch 😉).

tstoeckler → created an issue.

@arunsahijpal I took a look and the failures did look fairly strange, so I triggered a rebase which in turn triggers a new pipeline. Let's see...

Hit this as well, thanks for the patch!

Not exactly sure what you mean, per your endorsement I was → a core Entity API maintainer for quite a number of years but then stepped down 📌 Remove tstoeckler from MAINTAINERS.txt Fixed a while back in part due to just being less involved in general, but in part also because of the slow traction of even seemingly simple issues and also me not agreeing with the some of the focus/direction that things are headed in. Neither of those things have changed, so going back to being a maintainer is not really in question for me right now.

Oh, wow, thanks very much!

OK, coolio, opened 📌 Offering to co-maintain the module Active . Yeah, I totally get that, that was where I was at a couple years ago, as well, unfortunately (?) that's not where I'm at anymore ;-), but, yeah, at the end of the day, just let me know how you want it.

tstoeckler → created an issue.

On the concrete issue, one last pitch:
While this is technically adding lines of code, this (in my mind) really is just bringing over #2767857: Add destination to edit, delete, enable, disable links in entity list builders → to the duplicate operation so it's just adjusting contrib Entity API to core code drift. If you want to remove the duplicate operation (from this module) that's fine as well, and I can see why then it doesn't make sense to adjust it if the intent is to remove it anyway, but it would be great in that case to make a concrete decision along those lines then. (Whether in terms of code it just gets deprecated first or wholesale removed directly is an implementation detail from my point of view, for me the valuable thing would be the explicit intent of removal so I know where things stand.)

On the module maintenance:
Maybe that's best discussed elsewhere (and possibly through a more "synchronous" medium) but I'd be fine helping out or taking over the maintainership of the module, if and in whatever way that works for you. There's still a number of things I'd like to land here that I don't think would make it into core for various reasons so if you're looking to (soft-)"decomission" this module that would also of course be fine with me, then I'd just put my stuff in a different module namespace here on Drupal.org. Or we could put the 1.x here on life-support/soft code-freeze and I could put new stuff in a 2.x branch or whatever. Again, basically fine with whatever works best for you just wanted to put it out there if you are in fact looking for help.

I mean I guess it's somewhat opinionated either way, so feel free to "closed (won't fix)" this if you disagree, no hard feelings. I personally just like the UX of having the collection/list be a hub to which you return after each administrative action (i.e. concretely entity operation) that you start from that page. And given that premise, the duplicate operation is an outlier. In particular, if you think it would be best to land on the canonical/edit-form of the duplicated entity wouldn't it make sense to do the same thing when editing an entity?

Hey there, tested the last commit now, sorry for taking a bit.

I tested three scenarios:

1. No default scope in consumer and no scope in request -> { "error": "invalid_request", "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed.", "hint": "Check the `scope` parameter" }
2. No default scope in consumer and scope in request -> token with requested scope granted (again, this is the case I find suboptimal, but it's not a realistic production use-case anyway)
3. Default scope in consumer and no scope in request -> token with default scope granted

So looks perfect from my point of view

tstoeckler → created an issue.

Ahh OK, sorry for the noise in that case. No just noticed this issue and apparently I misread the (ancient) discussion above about is_file() vs. file_exists().

Is it correct that this means settings.php is no longer allowed to be a symlink? If so, would be nice to get a change notice for this. I do find that particular setup to be useful for local development, i.e. I have a generic settings.php that sets up e.g. the database name and things like that based on the site name and then (along with the respective webserver config) I can easily "spin up" an additional multi-site by just running mkdir sites/foo; ln -s '../settings.site.php' sites/foo/settings.php; and I'm off to the races. Although slightly less neat, not a big deal to change that setup to copy those files around, but, yeah, would be good to have an actual change notice for that, if that is in fact the case.

Opened a quick MR.

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

Ah, I didn't think about updating the example and test migrations to include the include key. Also will need an update path, I guess. Would love some confirmation, though, first that I'm on the right track with this before fixing the tests.

Opened a quick MR with the minimum to get this working. I opted to place the include key right before source, process and destination as I would guess that those are the keys that will be mostly "included", but I'm not at all tied to the order.

tstoeckler → created an issue.

Great work @arunsahijpal! I think this is quite close.

tstoeckler → created an issue.

That's fair enough. I think it's a bit unintuitive that if you do have a consumer with client credentials and no scopes initially and select a single scope it actually reduces the amount of possible scopes for that consumer. But, yes, that scenario will only really happen during initial setup or testing, it's not really a sensible "production" configuration to have a client credential consumer with no scopes. Maybe we could amend the description, though, to make people aware of this nuance? In any case, that was the only reservation I had so marking RTBC.

That's great regarding the module, I will open an issue for that, hopefully will get around to that this week, let's see.

Tried out the code now and it works exactly as expected. The tokens now automatically get the scopes selected for the client/consumer and any additional scopes are rejected.

I only have one question: When I was testing I initially set up my client to have no scopes at all. In that case this validation logic is not applied because it is within the

    if (!$client_drupal_entity->get('scopes')->isEmpty()) {

block. Even though it's not a very practical scenario, I would have expected the created tokens to have no scopes in that case instead of any requested scopes.

Will set to needs work for that.

But thanks again for working on this, really much appreciated.

One other aside, as I don't know where else to mention this: I built a tiny module that allows creating client credential tokens via the Drupal UI, to save administrators having to use Postman or whatever to do that. Since it's fairly generic I am planning to contribute it. I am presuming, though, since it's somewhat of a niche use-case that you wouldn't want to have that in Simple OAuth proper so I am planning on a separate little module. But wanted to check with you first, just in case that's something you'd be interested in having directly as part of the main module.

I added a review on Gitlab. I would think that the failing test is random, we'll see if it continues to fail when you update the code...

Thanks for picking this up, nice work!

tstoeckler → created an issue.

Hmm... I can reproduce the test failures in core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5DialogTest.php and core/modules/field_ui/tests/src/FunctionalJavascript/DisplayModeBundleSelectionTest.php, but I cannot reproduce it outside of a test. I even tried in Stark and the dialog appears perfectly with this branch. Not sure what's going on, maybe someone else can investigate, not sure I will get to it.

tstoeckler → created an issue. See original summary → .

Thanks for testing, that's strange, as I specifically tried this on a fresh install. But don't have the time to look into this further, so will close, I guess. And thanks for the pointer to the docs, that's very helpful!

Checked at least that the views operations still work (and the unit test passes), let's see if anything else breaks.

tstoeckler → created an issue.

Opened a quick MR that fixed things locally for me. Note that I did not test what happens with a sticky table header inside of a dialog, presumably it does not work correctly, but maybe we can look at that in a follow-up.

tstoeckler → created an issue.

Thank you so much for working on this! From looking at the code that is exactly what I had in mind, just wasn't sure you would be open to that change. That's really great 👍️

I had meant to test this last week, but that didn't work out and I'm off this week, but will try to prioritize checking this out next week.

Wow, thanks! That's awesome

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