🇪🇸Spain @penyaskito

I think this needs a explicit approval from @balintbrews and/or @jessebaker.

I'd love to know (and promote) Drupal CMS adoption, but IMHO we shouldn't do this:

• After initial creation of the site, any config changes or new modules installed make this "not Drupal CMS", because...
• as @catch mentioned in #8 this is supposed to be a starter kit.
• With the introduction of Site Templates, there's not a unique "Drupal CMS 2.0". I'd rather know the Site Template being used. And then this would become the responsibility of each Site Template, which we cannot force anyway?
• If we end up doing ✨ Add a basic but usable install profile for building a site from scratch Active , most Drupal installations will end up being "Drupal CMS" anyway. So in the future, what's the difference between Drupal or Drupal CMS?
• ... and in the meantime we are fragmenting numbers. Which would be a bad idea for marketing purposes.

Fixed IS syntax in case someone looks at this issue instead of the change record.

Pushed an attempt to test, but test-only is NOT failing.

I don't think we should block this on having a test, but if it's not a huge effort we should add one.

(categorizing as "Config management", don't think we have a better fit unless we use the general "Code")

I don't think we should update those docs BEFORE release.
I wonder if we took into account that docs might be different per version, so we can have different guides.

Thanks!

✨ Support "placeholder" dashboard blocks Needs review followup landed.

Merged the follow-up MR.

Opened MR with a fix. We definitely need a test for that.

This is the recipe fragment I used for testing this manually. A test should use that (so we actually test recipes can create placeholder blocks), + edit in the UI and verify it's still visible.

    dashboard.dashboard.welcome:
      addComponentToLayout:
        section: 0
        position: 10
        component:
          region:
            layout_twocol_section: 'second'
          default_region: content
          configuration:
            id: dashboard_placeholder
            decorates: dashboard_text_block
            label: 'Dashboard Placeholder Block via Config Action'
            label_display: 'visible'
            provider: 'dashboard'
            context_mapping: { }
            text:
              value: '<p>This block was added via a config action, and uses a placeholder</p>'
              format: 'basic_html'

Found out that the placeholder block cannot be edited, and the editor will actually lose it.
We're just losing the reference to the decorated block, not the data inside it, but because of that it will never display again.

Postpone on ✨ Support "placeholder" dashboard blocks Needs review , re-opened.

That was my intent, but Drupal CMS editors would be too confused if some blocks can be edited, and others cannot.
And I actually made this work :D Will open a new MR in the original issue, and leave this for just hiding the block from the UI.

Unrelated, but release blocking:

I'd expect you cannot edit the block. But actually the form works!
However, when you save it you actually lose the block!

@phenaproxima I'm guessing you have some recipe for adding placeholder blocks for testing. Can you test this, and/or even better, paste a recipe example here?
I want to ensure that they can ONLY be created through a recipe, and not in the UI.

penyaskito → created an issue.

Thanks for the patience!

This surfaced again in Dashboard's ✨ Support "placeholder" dashboard blocks Needs review .

Enabled automatic merge.

A problem I didn't think about before is what if the decorated plugin changes and needs an upgrade path.
But that's something that we have been innocently ignoring until now anyway, and probably depends on core's 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active .

True. I debugged this for a bit and the inputs are "lost" somewhere in \Drupal\canvas\Plugin\Canvas\ComponentSource\BlockComponent::submitBlockConfigurationForm, called from \Drupal\canvas\Plugin\Canvas\ComponentSource\BlockComponent::clientModelToInput

Tagging needs followup for https://git.drupalcode.org/project/canvas/-/merge_requests/451#note_650305, which is an extra improvement we might want cc @lauriii.

🎉 phpunit tests are green, let's hope for e2e.

I had to modify \Drupal\canvas\PropSource\StaticPropSource::isMinimalFieldItemRepresentation to keep collapsing list_string.
Before it was checking if there existed more than a prop. Now it checks if there is more than a non-computed prop.
But that affected datetime too!

Not sure of the consequences of that. But if there are, we need to tackle them, because list_string or datetime inputs would be different no matter if we decide to revert that change.

@mglaman and @penyaskito seal of approval, and @hooroomoo concerns responded, even if the outcome is not as great as it could be 🥲

Fix in place, needs tests. What a great bug 🐛, thanks all for reporting and being patient!

I can reproduce.

oh wait are you editing articles in the standard node form in both cases?
And Canvas is affecting that?

You can't edit node articles in Canvas, but Canvas pages (don't confuse with node Basic Page) or the Content templates for articles (which you have to publish to have any effect).

Can you update the steps to reproduce to be explicit about which case is this? Maybe you're checking node/1 instead of page/1?

penyaskito → created an issue.

Re-read the entire MR again, no concerns. 🚢 it!

Welcome to the date format adventure! 📌 Add date formats without time Active

At ✨ Allow mapping "List (integer)" field type to `type: integer` and "List (float)" to `type: float` Needs work , Wim wrote:

No such luck for "List (string)", because typically the values stored in there are "machine names", things such as "be_nl", "be_fr", "be_de" … — or "light", "dark" — or "red", "blue", "green". As such, they are clearly StringSemanticsConstraint::STRUCTURED. Which doesn't make sense to map to type: string, which are for StringSemanticsConstraint::PROSE. See commit 01088091 that introduced this — it predates the use of the d.o issue queue!

I can foresee very edge cases where someone might actually want the value, specially in code components. So even if out of scope here, I'd like to leave the door open for those. Which means we have something like

even if only "Label" is available now. Does this make sense, or we want to "flatten" this?

Thanks amateescu for stepping up!
As he is a subsystem maintainer for other subsystems I don't think this is required, but huge +1 for this.
Andrei is great to work with, and his experience with this topic is unmatched.

Created MR.

Created MR.

I can confirm:

• I have read and understand the definition of maintainers.
• I accept the Subsystem maintainer responsibilities.
• I am committed to following our community issue procedures and etiquette → paying particularly attention to providing constructive feedback → .

I can confirm:

• I have read and understand the definition of maintainers.
• I accept the Subsystem maintainer responsibilities.
• I am committed to following our community issue procedures and etiquette → paying particularly attention to providing constructive feedback → .

penyaskito → made their first commit to this issue’s fork.

Reviewed, we'll sync on this tomorrow.

Thanks!

I didn't really debug this, just intuition.

For the first one looks like just a presentation error, but the second one can trigger unnoticed changes (I actually doubted if this might be 2 different issues in d.org): e.g. If I edit any other prop and save, for the date + time the same exact error persists, but for the date-only one, each save will trigger a "day - 1" in the persisted date. Back to the past.

Created 🐛 off-by-one day errors + timezone errors when using SDC components with a date or date-time prop Active , thanks!

penyaskito → created an issue. See original summary → .

Discussed this today with @phenaproxima, and we agreed this can get in.

Added some nitpicks in the MR, mostly bike-shedding a name for this and being explicit about config dependencies being missing deliberately.

Thanks!

penyaskito → made their first commit to this issue’s fork.

Question about tagged services.

Thanks for the contributions! I was kinda frustrated today waiting for test results, so perfect timing for a bump in something that should make phpunit faster + sorry for taking over this issue.

Merged with HEAD instead of rebase (which is harder to review, but just faster, sorry!), so --verbose myself:

• We decided to keep 2 cases (one valid + one invalid) still as kernel tests, but the one we picked as of invalid, is now valid since 📌 Support designating a code component slot as the React component's `children` prop to allow JSX nesting Active landed! So I'm picking a different one (and that's part of the merge in 5389cdc1 for tests/src/Kernel/Config/ComponentValidationTest.php).
• db341ebc - "children" is a valid slot name now (same as above, but in the unit test)
• d06b1fcd - Test validating with a non-string value (aka a mapping) using the provider too.
• 15e85c40 - Simplify mock handling, we don't need the try-catch + ternary $is_invalid ? $this->once() : $this->never().

Related, and with some research in the comments: https://www.drupal.org/project/drupal_cms/issues/3521292 🐛 Tags in Blog Posts cause SQL error Active

But per #3, we might actually want it to be enabled. Which means the upgrade path defaults is undesired.
Given our shipped editors' config are locked and can't be edited by site builders, we might still want to merge that MR.

But definitely not a major priority issue now, and probably a minor..

We would need:

1. The config schema shim to ensure this passes tests with 11.2
2. Edit shipped editors' config
3. An upgrade path explicitly targeting our shipped config entity ids
4. A todo to remove the shim once we require 11.3

I missed that this was applied only to SDC, thought it was all SDC-alike.

Wim commits after #7 broke some JS Component tests, which means that wasn't the case. After fixing those inconsistencies, tests are back to green, so this should be ready to go.

👌🏽 double-checked that.

We used menus, and this was released in Drupal CMS 1.0.

And as this was specific for the Dashboard Track in Starshot aka Drupal CMS 1.0, we can close it.
Any issues we need in the future don't need a specific meta/plan.

Per #8, postponed on 📌 Validate Component config entity props' `required` value matches the actual source for the `active` version Active

penyaskito → created an issue. See original summary → .

Gave credit.

I considered this a bug tbh, and used 'fix' in conventional commits.

Thanks!

LGTM!

For awareness: ✨ Support the same block being used for page title & main content Needs review

I think we need some docs on the changes here. I tried that but not 100% sure I captured it on a comprehensive way.

penyaskito → made their first commit to this issue’s fork.

Wouldn't we want test coverage for this in the BE?

📌 Support adding a new optional prop to an in-use code component Active merged.

Thanks! Next: 📌 Support adding a new required prop with an example to an in-use code component Active

This is reduced now to prove it works after 🐛 Canvas' hook_storage_prop_shape_alter() must allow passing cache tags — to know when to re-evaluate Active landed. And tests are passing 😻

Pretty sure this is an issue with the provided content templates / missing components?
I'll try to verify that, but moving from Canvas to CMS for now.

Finally merged! Thanks all for the patience.

I wanted to ask component source codeowners before merging about the GeneratedFieldExplicitInputUxComponentSourceBase::getComponentPlugin() visibility changes, but I was able to not require those (I think what we needed was introduced after we started working on this).

So I feel more comfortable now merging this as this is only net new code, not doing a single change to existing classes.

#24 added the "Needs followup" tag for supporting blocks. But blocks probably are better by using the search_api provided "Rendered item" processor, so I don't agree we need any follow-up.

phpcs is complaining, otherwise looks good to me.

Thanks!

Some nits and questions.

Is this something that should have a test? Or not worth it?

If we ever do this, being so disruptive, let's think about DX while on it.

restrict access means nothing to me. I'm creating permissions, all of them restrict access to something? Why are _these_ special? That's obscure unless you look at the docs.

warning is a non-very well known and barely used custom message that can go with restrict access. I'd rather make that mandatory and have adequate description of what the actual danger is when granting the permissions.

🤩