Really could not figure out a proper way to do this, so I just opened 🐛 Test site may be destroyed while an (automated) cron is still running Active to explicitly work around this edge case and am adding that as a patch for CI. Seems to work, so that's going to be good enough until someone can come up with a better solution 🤷
tstoeckler → created an issue.
So the original fix for the automated cron thing that I had in mind (not storing the config object in the service, but the config factory) was in fact already fixed by 📌 Replace lazy service proxy generation with service closures Active . I was just looking at 10.4. Unfortunately that does not seem to completely fix the cron issue. From looking at the code I can't figure out why the automated cron still runs when the site is installed, but somehow it does... (or something else entirely is going on and I'm completely off track 🤷)
tstoeckler → created an issue. See original summary → .
Pipeline is green and here is the test-only job, which correctly fails on the missing table exception: https://git.drupalcode.org/issue/drupal-3540554/-/jobs/6158126
tstoeckler → created an issue.
Alrighty that was more "fun" that I had hoped for, but it's fixed now and I opened 📌 Improve test coverage to test the import code path Active as the follow-up. Marking fixed again and will create a release now with this.
tstoeckler → created an issue.
So looking closely
$targetStorage->read('core.extension')['language']
doesn't really make sense, it should be
$targetStorage->read('core.extension')['module']['language']
Found that out immediately when trying the new release. 🫤 Sorry about that!
I was so confident about this working because of the test coverage, so this exposes that the test coverage really is only testing the export code-path and not the import code-path - but for the "existing-config" tests (but I didn't add an existing-config test with language overrides here).
...so will add such a test now with the quick fix and then add a follow-up issue to improve the test coverage, to test both import and export in all cases.
Test-only job fails correctly, as the language override is not detected, and, thus, not removed on export: https://git.drupalcode.org/project/config_overlay/-/jobs/6008977
Don't love adding language-module specific code there, but a) didn't really feel worth factoring that out somehow at this point. We can always do that later b) language module is not really "just another module" and c) http://grep.xandeadx.ru/search?text=addCollections&filename= doesn't reveal any other usages of "addCollections" which is what is needed in order to support config installation of collections (regardless of Config Overlay). The only ones are Organic Groups Site Manager → and Variants → but neither of those have any releases at the time of writing.
tstoeckler → created an issue.
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.