Oops, that commit should have referenced 🐛 Mismatch of settings name in documentation and code Active instead of this issue.
Note that due to a copy & paste error, the commit for this one accidentally references 📌 Add test for $settings['config_overlay_priorities'] Active instead of the issue number here.
Just noticed that I already renamed a bunch of services in 2.4.0, so I guess it's unnecessary to introduce BC for this. 🤷
tstoeckler → created an issue.
OK, fixed this now including a little deprecation thingy. Did not write a change notice, but instead linking back here.
Also snuck a little cleanup in the MR to use strict types everywhere.
...and opened 📌 Add test for $settings['config_overlay_priorities'] Active for the follow-up to actually add some test coverage for this feature.
tstoeckler → created an issue.
Well that was a "fun" little rabbit hole. But now the test actually works properly and the test-only run still fails, see https://git.drupalcode.org/project/config_overlay/-/jobs/5947968
(For reference, that is the "test-only" job and it fails with:
1) Drupal\Tests\config_overlay\Functional\Integration\ConfigSplitTest::testConfigExport
Failed asserting that file "sites/simpletest/73257995/files/config_YbK6-VKVm4_WYUapjMfRtysCLDT4u4bXvjHNkWX0L2G2CK9OvPRBZ4UQ2xp9fYEM62b5KQEC0w/split/user.role.anonymous.yml" does not exist.
/builds/project/config_overlay/tests/src/Functional/Integration/ConfigSplitTest.php:176
)
Will merge now.
tstoeckler → created an issue.
Nicely failing test job: https://git.drupalcode.org/project/config_overlay/-/jobs/5937127
tstoeckler → created an issue.
That's interesting. Do you have 📌 Add an in-memory LRU cache Needs work applied in your project as well? I didn't validate this, but from the looks of it, this should automatically resolve the problem, as well, since the patch here is just using the existing memory cache and since that went in that should have the LRU logic.
heddn → credited tstoeckler → .
That's awesome, thanks a lot!
After 📌 Add a test with a profile with a config/sync directory Postponed this was now fairly straightforward to write a test for.
OK, looks good now.
Will add some docs for the config_sync_directory workaround over in
📌
Add explanation of how various scenarios are handled to README
Active
. Marking fixed.
Looked into this now. Core's InstallerConfigDirectoryTestBase really is a bit strange, at the very least for our use-case here. I realized that we were previously not really testing what I thought we were. At the very least you could previously write a test that seemingly would install Standard profile from existing configuration. That is currently not possible and only works in the test because InstallerConfigDirectoryTestBase creates a fake test profile in the site-specific profiles directory.
So will first open an MR to fix that which will hopefully allow adding some test coverage with a profile with a config/sync directory more easily next.
This was quite a ride. In the end we cannot support Config Ignore 1.x and 2.x because we cannot properly hook into the order at the right places with Config Filter and we cannot support Config Split 1.x because that in turn only supports Config Filter 1.x which does even allow us to run after any of the filters.
As part of this crazy adventure trying out various combinations I did end up simplifying things a bit by introducing the $settings['config_overlay_priorities'] setting and revamping the documentation quite a bit.
I guess I should point out that Config Filter 1.x is required for this bug to occur. Did not check specifically why.
tstoeckler → created an issue.
benjifisher → credited tstoeckler → .
Thanks @rkoller, I wasn't aware of that issue. Yes, totally makes sense to focus efforts there. Will see if I have some time to try dig into the issue there. But yes, let's close this then. Thanks for the heads-up! 👍️
Looked at this and in fact changing the priority from -100 to -50 to run before Config Ignore does fix the problem. Fix including test coverage incoming.
Should be fixed now https://git.drupalcode.org/project/config_overlay/-/commit/c3d88be252d65...
(forgot the issue reference in the commit message, that's why it doesn't show up here.
Will need to fix this in 8.x-1.x as well, to fix CI there.
OK, 2.x seems to be green now, now on to 8.x-1.x
Waited long enough to not have to bother with maintaining different versions, I guess...
Seems 📌 Move to InstallerConfigDirectoryTestBase Active is now a hard requirement for CI
Well we "just" switched the entire form from a single massive select field to an elaborate multi-step workflow, so I'm a bit baffled why now adapting the labels to fit into that new workflow vs. the old select list is somehow disruptive in the context of the larger change.
Hmmm... can you elaborate? Not sure what you're getting at specifically.
tstoeckler → created an issue.
Updating proposed resolution
tstoeckler → created an issue.
Thanks for sticking with this @prabha1997!
I tried it out locally and I guess my suggestion above was not really great. Turns out the inside of the if is reached. I had seen that the entity_test_constraints entity type has an actual changed field, but unfortunately that is not one of the entity types used in the test. (See \Drupal\entity_test\EntityTestHelper::getEntityTypes().) All of this is a bit of a mess, but not one we will have to fix here, I hope. So I looked into how to get an actual changed field into the entity types being tested without rewriting the entire test and found that the test entity types support adding base field definitions via state. I wasn't sure whether that actually worked as intended so I tried it out to be sure. And since I already had it then I went ahead and pushed it to the branch, I hope that was OK.
Maybe you can take a look and see if you agree with those changes, thanks!
Nice work with the test coverage.
Ahh, how annoying. I think that is because one of the test entities uses the changed_test item (i.e. \Drupal\entity_test\Plugin\Field\FieldType\ChangedTestItem) instead of the regular changed. I had missed that initially when looking into this. I guess we could also add the constraint to the test field type one, but that then kind of feels like testing the test coverage. Alternatively we could check that $entity->get('changed')->getFieldDefinition()->getType() is in fact changed (and not changed_test). I guess I like the latter solution a bit better, but also open to what you think.
Thanks for picking this up @annmarysruthy!
Looks perfect.
This will need some test coverage to go in, though. I looked around a bit and found \Drupal\KernelTests\Core\Entity\EntityValidationTest::checkValidation. I think it would make sense to add some checks to that as that already tests various field-type-specific constraints. All the tested entity types should also have a created field, so just adding a test for that should work fine. And for the changed field you can check if the entity type implements EntityChangedInterface as only a couple entity types have that.
Marking needs work for that.
I guess I was on an old state of the module without 🐛 No way to limit allowed scopes for a consumer using client credentialss Active so had forgotten that scopes are required now, so adapted for that. Should be green now (but for the test failure that is also on 6.0.x)
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:
- 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" } - 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)
- 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.