🇩🇪Germany @tstoeckler

@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

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".