phenaproxima → created an issue.
phenaproxima → created an issue.
phenaproxima → created an issue.
I formally accept co-maintainership of the recipe subsystem.
Sure, that works too! Done.
phenaproxima → created an issue.
A few small points, but in general this makes sense to me!
It will, however, need explicit test coverage.
phenaproxima → made their first commit to this issue’s fork.
This is very similar to what RecipeDiscovery used to do, but was reverted.
We probably want to adjust this slightly. If I understand the design intention correctly, are only two places that it should look -- the arbitrary $path that's passed to the constructor, and core/recipes. That's it. In other words, it should not hard-code to look in /recipes.
Which is what it already does in HEAD, to be clear -- but what we need here is an additional method that shows all recipes in those two paths.
Personally, I wouldn't follow ExtensionDiscovery's API here, purely because recipes aren't extensions and shouldn't be conceptualized or treated as such. But the ability to find them (to be exposed in Project Browser or other UIs) does seem handy.
We'll need to consult carefully with @alexpott on this one to be sure we don't introduce regressions, because the recipe system is very sensitive to changes in discovery.
phenaproxima → created an issue.
With the caveat that I don't understand the Svelte stuff (because I am not working in that space), this looks good and makes sense to me. This will absolutely enable recipe support in Project Browser, and we can do even better things once this is in. Avanti!
So about that...
It's not really true that you can't update label...it's just that there's no dedicated method for it on the ContactForm class. But this MR adds generalized access to ConfigEntityBase::set(), so you could do this:
contact.form.feedback:
set:
key: label
value: Personal contact form
...but that's sort of a lower-level action, so maybe a bit of an advanced use case.
All correct.
It's pretty much a subjective judgment call, but I only exposed methods that I think are likely to be useful, but not cause additional complications. Complicated things need their own issues and/or dedicated config actions, so we can think through and address their implications.
Correct.
phenaproxima → created an issue.
phenaproxima → created an issue.
phenaproxima → created an issue.
I think the issue summary seems pretty clear.
I'm putting this right at needs review because I don't think it would really stand to benefit from explicit test coverage, which would be complicated to write.
phenaproxima → made their first commit to this issue’s fork.
phenaproxima → made their first commit to this issue’s fork.
Okay, I think this is ready for a look.
I took a pretty broad approach to exposing config entity methods as actions, and added test coverage for every one of them. Some methods I omitted on purpose because they couldn't work from an API perspective, despite being useful (like \Drupal\image\Entity\ImageStyle::deleteImageEffect); others I skipped because their usefulness is dubious (like \Drupal\search\Entity\SearchPage::setPlugin).
Would be curious what people think.
Closing in favor of 📌 Determine which core config entity methods should be config actions Needs review .
Regarding #31.2: it's exported by the default_content module, which is what generates these YAML files. IMHO they should not be changed at this time, but if (which, once this is committed, is really when) core adds the ability to export content entities as YAML, then it may make sense to omit certain fields, like revision_translation_affected.
Sort of beside the question, but...never update the dependencies key using simple_config_update (or anything else). Config dependencies are computed when the entity is saved; they're meant to be managed by Drupal.
Other than that, your recipe looks good. :)
Alright, I think this solves the problem reasonably well. Let's ship it.
This looks like what we agreed on.
The only thing I think is commit-blocking here is a strongly explained reason why discovery is not centralized, and why core is special-cased. Without that, someone (maybe one of us) is liable to come back to this code in a couple of years and try to add dynamic path resolution or other stuff which would break the intention here.
We're intentionally limiting it as a reasoned design decision, so we need to be absolutely 1000% crystal-clear that the decision is explained.
Yes - it means things in core need to change.
But that's not in scope of the recipe system. (I had raised this same concern with @alexpott.)
Why is that interesting, though?
The human-readable name is just that -- a human-readable name. In combination with the type key, there's nothing obviously special about it. It can be as descriptive -- or, more likely, undescriptive -- as the recipe author wants. There are no guidelines, and certainly no rules, about uniqueness (if there, we'd need to explicitly validate the name's uniqueness, at the very least).
Recipes are really only uniquely identified by their path. Since they are not extensions, it would be legal for them to have the same directory name (machine name). Since the code in question needs to identify recipes unambiguously, the path seems like the cleanest way to do that. (We could also hash the contents of recipe.yml but that might be overkill.)
phenaproxima → created an issue.
phenaproxima → created an issue.
Makes sense to me. RTBC once all tests (and the validatable config job!) pass.
One question, but otherwise that looks RTBC to me. I agree that there's no clear need for an update path.
phenaproxima → made their first commit to this issue’s fork.
I reviewed the code here and didn't find anything I'd consider commit-blocking. There are some minor phrasing things, comment nits, and stylistic suggestions, but overall I'm seconding the RTBC here.
I also really like the idea of #13. set() has its own problems, but it makes perfect sense and will be useful in many, many situations.
This mostly makes sense to me. I think it feels a little messy, but since this is all internal, we're free to refactor it later.
phenaproxima → created an issue.
Re-titling for clarity of scope.
Okay, wrote a test for this.
I think that pretty much covers it; we're not actually trying to fix the validation error itself, just the fact that the original exception is swallowed.
I agree on the RTBC, but it looks this needs a rebase/merge against the target branch.
phenaproxima → created an issue.
We can definitely detect if it's a config entity (I've been using \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeIdByName() for that). If there's a way to map to an entity from the config record, and then bring in the entity API, I'd rather use that -- best of all worlds.
I'm happy with this, but it looks like it needs a merge/rebase against the target branch.
One minor typo thing, but otherwise RTBC!
Assigning to @tedbow for review on !1063.
phenaproxima → made their first commit to this issue’s fork.
A few relatively minor suggestions...
One tiny suggestion but otherwise this is RTBC.
I suggest a few changes (mainly an example, for clarity) when describing the dynamic nature of the action.
Couple of minor suggestions -- feel free to accept or reject them, but otherwise I think this reads great and is RTBC.
How would a recipe creator alter config entities then?
A module -- either Metatag itself, or a custom module -- would need to provide the appropriate config action(s).
The problem with simple_config_update is that it's a crutch. It also bypasses things like entity save hooks and entity-level validation, which may well be providing critical under-the-hood functionality and sanity checking. So doing a simple_config_update on a config entity right now may appear to work, but have secondary side effects that aren't discovered until later, at which point they'll be nearly impossible to debug.
Note that this is going to be a BC-breaking change for early adopters of the recipe system. It's common to use simple_config_update to change config entities in ways for which config actions don't yet exist.
So it might be preferable to raise a deprecation here instead in the 10.x branches, and throw in the 11.x branch. But I leave that determination to @alexpott.
phenaproxima → made their first commit to this issue’s fork.
Pretty sure this one is blocking #3442022: [PP-1] Trigger entity validation in \Drupal\Core\DefaultContent\Importer::importContent() → .
I suspect the test failures here are linked to #3442024: Account switching to user 1 is now fraught → , so postponing this issue on that one.
+1 from me. This feels like table stakes for any file-based media type. I think deleting the associated file should be the default behavior; if it's not in the CMS, it doesn't make sense (from my own personal user perspective) for it to persist in the filesystem.
OK, with some guidance from @saschaeggi I was able to confirm that, with Gin Toolbar installed, the shortcuts always appear in the top bar, even in the frontend theme. So indeed, no inconsistencies if everything's set up correctly.
As far as I can tell, this patch is good to go!
Makes sense if it's an intentional design decision.
But the problem I'm having is the inconsistency. After all, the navigation will still appear when the front-end theme is active; elements should not move out of the navigation when viewing it in the frontend theme, because the navigation is fundamentally an administrative element of the page, regardless of theme.
Is there a way to make Gin not move the shortcuts around? If not, could that be added as a setting?
Ah -- never mind. Gin moves the Shortcuts list to the top bar, which is a legitimate design decision.
Okay, I can't find anything to fault!
I tested this out and I see one glaring difference between Gin 3.x-dev with this patch, and Claro:
With Claro/Olivero:
With Gin:
As you can see, Gin is for some reason not displaying the Shortcuts sub-menu. That's important for my project. Am I missing something here, or is there a bug in Gin?
phenaproxima → made their first commit to this issue’s fork.
This was tricky, and revealed a flaw in our handling of duplicate file entities.
Turns out file entities cannot be duplicated by URI. That's validated at the entity level. Yet we specifically test for that -- the only reason that test passed before, is because we weren't doing entity validation.
So I refactored the file handling logic to do this, in order:
- If there's no physical source file to be copied, there's nothing we can do. (This is the existing behavior.)
- If the source file and destination file exist, but have different contents, then ensure the destination file will have a different name. (This is the existing behavior, changed slightly.)
- Now that we know the destination URI, check if there's a file entity that already has it. If there is, then don't import the incoming file entity -- just use the existing one, and ensure that any other entities being imported in the current batch which reference the new file entity, will actually reference the existing one.
- Finally, if there is no file entity for the destination URI, copy the physical file and update the URI of the file entity we're importing.
The test coverage probably has some gaps, and I do need to write explicit coverage that validation is triggered, but this is progress...
phenaproxima → made their first commit to this issue’s fork.
Why is this core MVP?
Deduplication isn't really necessary, as far as I know; the result of these methods is used to check if they contain a specific value; we don't really care (yet) how many times they appear. So while I don't necessarily object to using array_unique(), I'm not sure this actually needs to be core MVP.
I'm not sure we need this. The entity system will throw an \InvalidArgumentException, after all, if the field doesn't exist.
The only real benefit here is that we could throw a more clearly worded/contextualized ImportException. But that doesn't feel like core MVP material to me.
phenaproxima → made their first commit to this issue’s fork.
I didn't see an existing issue for this in Default Content, so I've fixed it and added test coverage.
phenaproxima → made their first commit to this issue’s fork.
phenaproxima → created an issue.
Okay, I'm going to guess this is therefore RTBC once tests pass!
I think we need to make it clear that this works for any config entity type that carries third-party settings (which, by default, they all do).
Some minor suggestions but it looks good to me otherwise.
Good to go if tests pass.
It worked okay for me.
I did it this way:
drush si minimal -y
# Apply the basic_shortcuts recipe directly
php core/scripts/drupal recipe core/recipes/basic_shortcuts
# Now apply the Standard recipe, which also runs basic_shortcuts
php core/scripts/drupal recipe core/recipes/standard
That worked fine.
Conversely, if I check out 11.x and do the same command sequence, I get the expected SQL error:
SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint faile
d: shortcut.uuid: INSERT INTO "shortcut" ("shortcut_set", "uuid", "langcode
") VALUES (?, ?, ?); Array
(
[0] => default
[1] => 478b3170-1dfd-49d8-8eb3-f1b285445ab7
[2] => en
)
So, as far as I can tell, this MR fixes the problem.
Too bad you reverted and went with a custom constraint in the end
Yeah, I was disappointed I had to revert also. But I realized that there was a very subtle bug in my original approach, just waiting to give somebody a splitting headache. The problem is that getAllCountryCodes() is not part of CountryManagerInterface. So the validation constraint was coupled to a non-interface service method.
If someone (or some module) swapped out core's CountryManager for a different implementation of CountryManagerInterface -- one without the validation method -- they would have run into some very obscure, hard-to-trace errors.
In light of that, I decided it was best to go with a custom constraint that wraps specifically around CountryManagerInterface::getList(), which is an interface method and therefore won't break if the implementation is swapped out.
I agree that making Choice aware of the callable resolver is likely to be useful in the future, and as soon as we have a concrete case for it, I'll happily restore it. :)