phenaproxima → created an issue.
phenaproxima → created an issue.
phenaproxima → created an issue.
phenaproxima → created an issue.
This is now fixed thanks to the upstream bug fix, and is tested here: https://git.drupalcode.org/project/byte/-/blob/1.x/tests/src/Functional/...
...and it no longer does. This was fixed in 🐛 Adjust Byte's dependencies to account for changes in Drupal CMS Fixed .
Merged into 2.x and cherry-picked to 2.0.x.
phenaproxima → created an issue.
Merged into 2.x and cherry-picked to 2.0.x.
phenaproxima → created an issue.
Fixed this directly in ec5a83942b3cc72f3ff212c83eb8b768354b3a62.
This needs a change record for the 1.2.x filename sanitization improvement.
I implemented whitespace replacement over in 📌 Ship local video media type with Drupal CMS for better compatibility with Canvas Active , but not full transliteration and non-alphanumeric replacement. Should we proceed with that? I'd love to get sign-off from someone a little more experienced in multilingual matters.
Merged into 2.x and cherry-picked to 2.0.x.
We might be able to fix that by having this implement a Composer package alias for both drupal/drupal_cms_local_video and drupal/drupal_cms_video. I'll have to check with @drumm on that, though, so that will be a separate issue.
phenaproxima → created an issue.
Fixing this directly is not in Drupal CMS's bailiwick.
I think this would be useful and good.
This was implemented by ✨ Drupal CMS variant Active !
The listing pages have been removed from Drupal CMS 2.x, so this is a won't fix.
This situation has changed; in Drupal CMS 2.x, the breakpoints are now defined by an ECA model and are therefore completely customizable, not tied to any specific theme (although the current breakpoints are borrowed from Olivero). So I think that we can at least say this issue has fulfilled its purpose, and further iteration/improvement can take place in other issues.
This is already implemented; Byte and Starter each have their own version of recommended.yml and there is a clear pattern for doing it similarly in other site templates.
We no longer have any JavaScript-based E2E tests, so closing this one as outdated.
This is very easy; all we need is an appropriately-sized image.
Assigning to @pameeela for review and sign-off.
I just merged ✨ Add ComponentSourceInterface::getReferencedEntities Active into Canvas, which implements proper export for entity references that are prop inputs. Closing this one as a duplicate of that.
That makes a lot of sense to me.
Auto-merged into 2.x and cherry-picked to 2.0.x.
@pameeela approved this verbally over Slack. To summarize the decision: we will continue to have the content types, but we'll remove the listing pages entirely and remove all Layout Builder-managed information. This means the content types will ship with simple lists of fields (the old "Manage Display" way), but that's okay -- in Drupal CMS 2, the intention is to use these content models if they're convenient for your use case, but otherwise to define the display of them in Canvas, combined with a design system.
So this achieves the goal of removing Layout Builder from Drupal CMS's content layer, but otherwise keeps the (minimally useful) content types as they are. This has the happy side effect of relaxing the dependency tree a bit; there's no more need for every content type to also depend on the Page content type.
Merged into 2.x. This does improve performance, although not by a ton, but enough to make a difference.
One-line change and an extremely easy-to-write test, thanks to https://www.drupal.org/node/3553794 → !
I had not! Thanks for the pointed reminder. :)
I guess that rationale makes sense, although default content is never "syncing" as such. It's always getting created anew; we're always calling enforceIsNew(). It's a weirdly named method, frankly.
But I think I can see the benefit and purpose here. It won't harm anything.
I'm not clear on what the benefit of doing this would be...? Could that be fleshed out a bit in the issue summary?
I personally don't think we should do this; the cons outweigh the pros.
While I agree that "depends" is a less clear key than "dependencies", it's not that much less clear, and keeping it as "depends" retains compatibility with all previously exported content. If we changed this key, we'd be forcing everyone to re-export their content for Drupal 12 compatibility...in exchange for a pretty marginal benefit.
After all, default content is not human-readable; it's not something you're meant to craft by hand. If you are savvy enough to be hand-editing default content, I'd bet that you can infer that depends is referring to dependencies. (We could document that somewhere to make it explicit.)
So, for me, I'd need to see a more pressing reason to change this key in order to push this forward. Therefore, I'm tentatively closing this as a "works as designed", but feel free to reopen if there's more discussion to be had. I may well be missing something.
I don't necessarily object to this on principle, but I'm curious what the concrete use cases are. It is very, very easy to programmatically import content (in a module's hook_install, say):
use Drupal\Core\DefaultContent\Finder;
use Drupal\Core\DefaultContent\Importer;
function MYMODULE_install() {
$finder = new Finder(__DIR__ . '/content');
Drupal::service(Importer::class)->importContent($finder);
}
That's pretty lightweight, and it gives us more flexibility to let the module decide when content should be imported, rather than core having made that decision.
So, for me, we should document the above code somewhere and then close this as a "won't fix".
I'm happy with this!
Issue summary looks pretty up to date. We might need a change record here (although this does not change existing API at all) but if we do, we'll let the committers tell us about it.
That looks pretty good but maybe we could add dedicated test coverage in a follow-up? If we could get, say, a snapshot of 50 recipes from drupal.org as a JSON file, we could surely have a test to put this plugin through its paces.
No worries, @mparker17. This is a Drupal CMS stable release blocker, but I have a workaround in place in our helper module so it's not an immediate emergency. Thanks for your incredibly responsive and responsible maintainership!
Looks legit to me! We could add tests in a follow-up.
I love this idea, but I don't think we should do it as a separate attribute. I think we can add a couple of properties to existing attributes, and derive everything else.
Short label
Already exists in both ActionMethod and ConfigAction, called admin_label.
Long description
This could be derived from the doc comment of the method (for ActionMethod) or the class's doc comment (for ConfigAction).
Applies to
This can be inferred for ActionMethod, which is already part of specific entity types by definition. ConfigAction already has an entity_types key which serves this purpose, although we could expand its scope to include config schema types, so that you could put something like config_object to explicitly state that the action applies to simple config.
Patterns
Both ActionMethod and ConfigAction should support full, explicit, array-encoded usage examples -- much like SDC props do.
Plural label
This is inferrable for ActionMethod, and ConfigAction plugins with derives can just expose full documentation for each derivative.
Available since
We should add a property for this to both ActionMethod and ConfigAction.
No objections here! This is partially blocking progress in Drupal CMS so let's ship it, I say!
Looks right to me, and does what the issue summary asks for. One small nit with regard to whitespace at the end of the class, but that could be fixed on commit (I think).
Per Alex in #15, I have reduced the scope a bit to still only support the canonical link template, but to allow URL options (certain ones) to be passed in. There is test coverage that absolute and relative URLs are supported via the absolute option, which defaults to TRUE in other to be broadly compatible with uri and uri-reference formats.
What else needs to be done here?
phenaproxima → made their first commit to this issue’s fork.
Ah yes, thanks for reminding me. Implemented! Assigning to @pameeela for manual testing and review.
Fixed the remaining things and added ignore missing to fail gracefully.
Tested this and confirmed that the UX is now correct, and the props seem to behave as expected. Assigning to Bálint for review.
I'm manually testing every single altered prop to ensure they have the desired UX.
As far as I can tell, all feedback is addressed and missing test coverage has been implemented.
That looks shippable to me. The update path is a tad squirrelly and I was initially going to ask why we need a whole-ass event subscriber for it, but then I realized it's because it would be possible to set it back to an empty string at any point, even after the update path did its job. But how nice it is to dump a bunch of workaround code from SchemaTestTrait!
This should be sorted out before Mercury is stable. Tagging accordingly.
I think this is a stable blocker, since we want images to be rendered consistently in Mercury-based themes. Because Drupal CMS's stable release requires Mercury to be stable as well, it's a stable blocker for Drupal CMS too.
This blocks stable releases of Mercury and Drupal CMS.
In reviewing the MR, I have to admit I don't really understand why it's implemented how it is. My main question is this:
Why are the event information stored in local config? That makes it not updatable, at all, without an upgrade path (i.e. hook_update_N).
Is the idea here to have a feed of events that are coming up in your area? If so, is there already a page somewhere we could link to? If so, that would be a lot easier -- we could define a couple of links that get added into the administrative navigation upon install, for example, much the way that the module already does. That'd be quick and easy, and it looks like it'd fulfill the use case (maybe an "Events near you" link and a "Find your local user group" link).
UX-wise, we could even create a new subsection of the navigation, called "Community" or something like that, to house those links. That might be an even better use of this module, and a nice way to implement the ideas here. It might help, at this point, to ask a UX person for some guidance.
Pam also confirmed for me in Slack that she would have no particular objection to adding this to Drupal CMS, once it's implemented properly in this module (however we go about doing that). That would be still need to be in a separate issue with her explicit sign-off.
I have the most experience with this subsystem so I'll deal with this one.
Yeah, that's what I would do as well.
In fact, this issue collides with that critical fairly head-on, so let's block this on that one.
This is a question for Bálint or Lanny, but as a matter of future-proofing, is it possible we could configure Tailwind such that it never scans for utility class names in *.yml files? That would be a fairly strong guardrail against this regressing.
Well that was a slow, boring few hours but I think I got all the hg: stuff out of the components.
This did call for some refactoring in the Twig templates; I think I got it right but I would love review from Bálint and Lanny to make sure that a) my approach was kosher, and b) that I didn't miss anything obvious.
Unfortunately we cannot merge this as-is because it further entrenches the release-blocking anti-pattern that we're trying to stamp out over in
🐛
Do not use Tailwind CSS class names in prop enum values
Active
. So this needs to be refactored a bit to at least remove the Tailwind classes from *.component.yml files.
Emphasizing that we cannot release another tag of Mercury, at any stability, until this is corrected.
Oh shit. 😱
I'm glad that not too many components are affected by this, because this is a release-blocking critical show-stopper. This is problematic enough to make me "ground stop" even another alpha tag of Mercury, let alone a stable release of Drupal CMS, until it is fixed.
Bálint is correct -- component props' metadata, like enum information, is directly tied into the Canvas data model, at the very least in the form of the version hash. Tying styling information into the data model is an expressway to hell and needs to be fixed.
Everything done in this MR seems reasonable to me, but it does need a quick manual test from Pam, in the context of Byte, to make sure we didn't accidentally break any styling.
RTBC from a code review standpoint, but should not be committed until Pam confirms it's good.
There's no need for explicit test coverage here; the removal of created from the default content fixture is sufficient, because ContentExportTest already asserts that the exported content is identical to what's in the fixture.
Great! This will need a follow-up issue, then, specifically to add this block to Drupal CMS's dashboard -- that decision is up to Pam, so the best choice would be to open the issue without a merge request until she explicitly signs off.
If we're going to do this, it should not be happening in Drupal CMS Helper.
That module is meant specifically to polyfill missing pieces from core and contrib; providing a block is completely out of its scope.
I would instead suggest that we add it to Drupal Association Extras → , which is precisely the kind of place that a block like this would make sense.
The main benefit is that it won't be up to Pam whether to add this block or not; instead, the decision would be made by @hestenet and DA folks. Pam would decide whether Drupal CMS should expose the block (i.e., by adding Drupal Association Extras as a dependency).
So I'm going to move this issue over that module's issue queue, then assign to @hestenet for input on whether he thinks this feature should be accepted.
Missed a spot here; if the default content system is fully polyfilled, then there is no need to register the event subscriber conditionally. And there is a flaw that needs to be corrected -- the event subscriber will break, outright, if Canvas isn't enabled.
I would recommend we hold off on this until the product owner (@pameeela) approves doing it. Otherwise we're doing work that may not be merged. :(
No complaints here. Started a merge train for 8.x-2.x. Thanks!