🇺🇸United States @phenaproxima

Self-assigning to squash this bug with test coverage.

phenaproxima → created an issue.

Alright - merged into 0.x. Let's get this improved in core!

I've seen this too. It's not installer-related, there's something wonky in a recipe somewhere. Pretty sure it's the dashboard one, so categorizing this issue under that track until it's proven that it's innocent!

Yeah - the idea is to merge this now as an incremental improvement, then rely on the change being made in that issue once it lands in a tagged release of core. 👍

Okay - I merged !213, and will tag beta1 from that, but that's not the end of this issue. Let's deal with the rest of the dependencies/patches in subsequent MRs in this issue.

On second thought, let's do this in 📌 Switch to tagged releases of all dependencies Active , which covers the scope of this.

phenaproxima → created an issue.

OK, so all PHP tests are now passing but Nightwatch tests are failing because they need to manually add the body field after creating a new content type. I have no Nightwatch experience.

Alright, started an MR. Let's see what breaks.

With heavy heart, merged into 0.x.

Assigning to @thejimbirch for final review. I think this is a track-level decision so not going to wait on product approval.

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

phenaproxima → created an issue.

Okay, great. So the next steps here:

• Add these recipes as dependencies to project_template/composer.json
• Add a little test coverage to the starter recipe which confirms that they all apply on top of it

Now that the decision has been made, this can land during beta.

Merged into 0.x. I look forward to restoring Tour one day when it's a little more ready for prime time!

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

The tests were failing due to a core bug - I've filed 🐛 The PlaceBlock config action doesn't account for the possibility of there being no other blocks in the region Active to deal with that separately. In the meantime, it's unlikely to affect real-world sites, and can be worked around in the test. So that's what I did.

This is ready for final review by @pameeela!

Merged into 0.x - great fix, thanks!

Looks great.

phenaproxima → created an issue.

From a code review perspective, I think this looks excellent. Tests are failing, though - chances are there is an idempotency bug. Kicking back for that.

Merged into 0.x. Thanks all!

Merged into 0.x. Thanks!

Actually, though...on second thought, the patch is right-minded, and would indeed be necessary for the privacy recipe to work independently.

Can you convert it to a merge request, please?

There's no need for a patch. It works as designed. If you want to spin up Drupal CMS just to try it out, use the instructions at https://git.drupalcode.org/project/drupal_cms/-/wikis/Usage.

phenaproxima → created an issue.

Here's an idea - what if this plugin was a wrapper around three common array functions -- array_push() (add to an array), array_unshift() (prepend to an array), and array_splice() (insert into an array at a particular position).

It could honestly be a pretty simple wrapper, where you literally just pass the arguments to the function:

'simpleConfigArray:append': # Calls array_push()
  property: path.to.property
  values: [add, these, to, the, array]

'simpleConfigArray:prepend': # Calls array_unshift()
  property: path.to.property
  values: [put, at, the, beginning, of, the, array]

'simpleConfigArray:splice': # Calls array_splice()
  property: path.to.property
  offset: 1
  length: null
  replacement: [values, to, insert]

This could truly all be implemented in a single plugin (simpleConfigArray) with a deriver that generates the three variants.

Ultimately we have decided not to do this.

There are some branding issues about it, some infrastructure blockers, and although we don't dislike this idea, it's not important enough to pursue. We have bigger fish to fry.

I am not sure about this as proposed.

I think we should lay the foundation here for a more generalized array manipulator, since there are many operations we might want to do on arrays in simple config. I've put an idea or two in the MR.

So I reviewed this and I actually think it looks pretty good! Here are a few major suggestions:

• Your patches are not being applied because you need to add drupal/drupal_cms_search to project_template/composer.json as a dependency. Do that, and then run ddev rebuild, and you should be fully patched.
• We are most likely going to remove the drupal_cms_search_filters recipe due to Facets not being D11 compatible yet. It's not in scope for this issue anyway, so it should be removed and put into another issue/MR. Can you do that?
• "drupal_cms" is not a descriptive or helpful machine name for the search server or index.
• The test coverage is admirable, but it also is not really testing anything valuable beyond the recipes' idempotency. What would be best, IMHO, is to add a single end-to-end Cypress test to drupal_cms_search which does something like:
• log in as a content editor and add some nodes: one included in the index and published, one indexed but not published, one published and excluded, one unpublished and excluded
• run cron
• log out
• confirm that the published, non-excluded nodes is found when you search for them

To me, that would prove that the recipe is actually delivering what is intended. We could add this test later in a follow-up so as not to block commit; the idempotency tests are enough to get this merged.

• @breidert explained to me that drupal_cms_search_base is split out from drupal_cms_search in order to facilitate decoupled searching later. I think that's something we can consider for the future, but we should not do it now; we should just have a single drupal_cms_search recipe with all of this stuff. We can split it out later as demand dictates.

Okay, there are two options here:

1. Keep the three recipes in contrib, and bring them into the project template as dependencies.
2. Merge them wholesale into Drupal CMS, and deprecate the contrib projects.

Either way, we can show them in Project Browser. I like door #2. @tim.plunkett is agnostic. It sounds like Pam is more on the door #2 side.

I think @mandclu must choose.

I discussed this with @alexpott in Zoom today.

Long story short: this issue can proceed as it currently exists, but there needs to be a longer-term plan to unify the messy solar system that consists of Typed Data, config schema, the form system, and CLI input. Drupal doesn't have a single generalized "data input, validation, and translation system" that we could leverage, really. The recipe system leverages Typed Data in a reasonably pure way, but as the current patch demonstrates, that necessitates encoding form API elements into recipes if we want them to be presented in a particular way in a particular context. It would be so much better if I could just say "this input is a string with these validation constraints", and trust that Drupal would be able to present that input appropriately in the CLI, in a form, etc.

But we're a long way off from that.

So we decided to be pragmatic as ever, and implement the proposed approach. Hopefully we can deprecate it as core's ability to handle input in a unified way becomes more robust. But for now, this is the clearest way for us to deliver this essential feature.

Can we add some end-to-end test coverage to the SEO Tools recipe to ensure that you can load the node forms without errors?

Some minor stuff, and there are out of scope changes that should probably be reverted. But overall I don't see any major problems with this code.

Found something that is suspicious at best, wrong at worst, and probably needs test coverage. But I may be wrong. If I am, great - let's just add a comment to clarify and it's full steam ahead.

OK - I think this looks good. Not sure if @pameeela needs to review this, since we paired on it, but assigning to her anyway to give the final thumbs-up.

Changing status; "needs review" is usually meant to be used when there is a merge request to look at. )

Sure, that sounds good. I'll adjust my comment.

@tonypaulbarker and I discussed this recipe on Zoom and how we think it should proceed. Here's what we landed on:

• We don't really need a media_base recipe. The only things that are shared between images and remote videos are the small, medium, and large view modes. I think it's perfectly okay for those to be duplicated if necessary.
• Let's have two recipes here: drupal_cms_image and drupal_cms_video.
• drupal_cms_images should be a maximalist recipe that contains everything needed for images to work as editors would expect them to. That means all image styles, crop types, responsive styles, view modes, etc. are contained in this recipe. It provides both the Image and SVG Image media types. Ideally those would be only one media type, but Tony explained that there are technical limitations around cropping that prevent that from working as we'd like. It's the kind of thing we could evolve in future versions of this recipe.
• drupal_cms_video will be a wrapper around core's remote_video_media_type recipe, adding the small, medium, and large view modes.
• The starter recipe will include core's audio_media_type recipe as-is, along with drupal_cms_image and drupal_cms_video.
• The starter recipe will also expose core's local_video_media_type recipe to Project Browser, for sites that want locally hosted video.

So, kicking back to "needs work" to reorganize this as we agreed. But truly this is magnificent and robust - I'm amazed by the diligence and thoughtfulness that went into this.

Well...it's not hardcoded, exactly; it looks like it's a layout section, so once Layout Builder has config actions to manipulate a section, we should be able to remove it that way.

Oh, dang. Well, glad you caught that!

User is a required core module and is always installed on every Drupal site.

Assigning to Jim to debug this one.

Reviewed this and I have two overarching concerns, one of which is commit-blocking and one of which isn't.

The blocking one is that media_base is constructed as an anti-pattern. It should not include any media types of its own; only things which may be needed by all media types. We probably will want the image media type to have its own recipe, built on top of media_base.

The non-blocking one is ZOMG I have never seen this many image styles in my entire life. Won't marketers find this overwhelming? This is an enormous amount of config, choices, and (seemingly) UI complexity to be introducing. How do we expect people to use this? I don't build sites for a living so I may very well be coming at this wrong, but this seems like it will be very intimidating for people. Should we streamline it?

🤔 Pam told me that was changed to admin_only by default in core recently, so that's why removed that line.

OK - Pam and I paired on this today and now it's just a question of getting the tests to pass. Self-assigning to figure that out.

Thanks! Merged into 0.x.

@alexpott and I discussed that and we feel that it does not because it’s completely internal refactoring, and it’s not clear how we would go about testing this anyway.

Looks like we can't do this unless we can also take over the cms_events project namespace. That project is abandoned so I imagine it won't be a problem, but it's a hard blocker to doing this.

One small change requested. The MR is still in draft, though; should that be changed? There also seem to be merge conflicts against 0.x that need to be resolved.

Looks good to me from a code perspective; assigning to @pameeela for final review.

Idea: what if we changed the installer such that:

• Turned the timezone selection into a hidden field, whose default value is the server-detected timezone
• Use some JavaScript to ask the browser what timezone it is, and update the value of that hidden field
• Set Drupal's timezone to whatever that hidden field submits

Can't believe how long that took to get right; turns out Help was baked in to a fair few spots. But, that's what we have tests for. Merged into 0.x.

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

I only just became aware of this issue today, but from chatter in Slack I suspect that the one with the new hooks is NOT the one they are aiming to merge.

phenaproxima → created an issue.

Added to the wiki: https://git.drupalcode.org/project/drupal_cms/-/wikis/Architecture-Decis...

Okay, I think this is pretty much where it needs to be. Reassigning to @pameeela for final review.

I don't think this should be a beta blocker.

That's not to say the status checks are slow, 'cause they are. But they're not so slow that they're, like, consuming gigabytes of RAM and all of the server's CPU resources. So this is more just something that sucks, and it does need to be addressed, but it's not a critical thing that needs to block a beta.

This is a Drupal CMS stable blocker.

Right now we are patching core to get this functionality, but core is a moving target and the patch needs to be rerolled often. This makes it unsafe to ship in a release of Drupal CMS. According to @penyaskito, getting this issue in would fix the problem once and for all. That makes it critically important to land in 11.1 for Drupal CMS.

I ran into a core bug. If you apply content_type_base on Standard without any media types existing, you will get this error when you try to add content:

The website encountered an unexpected error. Try again later.

InvalidArgumentException: The allowed types parameter is required and must be an array of strings. in Drupal\media_library\MediaLibraryState->validateRequiredParameters() (line 151 of core/modules/media_library/src/MediaLibraryState.php).

This is unambiguously a bug in core. The MediaLibraryState object should not be resilient to the possibility that no media types exist. We'll have to open an issue for that.

In the meantime, this is probably not a commit blocker or stable blocker, because we don't normally expect folks to apply drupal_cms_content_type_base on top of Standard, although technically we need to support it. The workaround is probably to get the tests passing by applying a media type recipe (drupal_cms_image_media_type) when setting up the test site.

Yet another reroll on 11.x.

OK - that was a hell of a rebase but I got it done.

It turns out my plan in #3 won't work as expected due to some technical stuff in the Filter module (which is frankly a weird mess and needs to be heavily refactored, but that's not for us to do).

I now think what makes the most sense here, especially in light of the very clear "Content" naming, and the fact that there are also an "Email" and "Webform" formats, is to ship the new format in drupal_cms_content_type_base. That way we know it's available to all content types, since they all depend on that base recipe.

Have you tried ddev rebuild? Usually that'll clear up errors like that one.

I don't think we should necessarily add a test for this because it only would make sense within DDEV, so you'd have to remember to run the test only within a DDEV context. And sure, we could add a test, but since it wouldn't work on CI, what would be the point?

What I suspect is happening here is that another one of the recipes in the starter's stack is applying core's basic_html recipe (which is necessary in many cases for default content). Since the starter doesn't use config actions to set up its extensions to basic_html, but instead ships it wholesale in the config directory, it gets ignored so as not to conflict with the already-existing config.

What needs to happen here is one of two things:

The starter (or some other recipe in its stack) needs to use config actions to alter the basic_html format from core.
The starter needs to ship a different format that has the needed changes - this is the approach being taken in 📌 Create a new text format/editor to replace core's Basic HTML Active .

A solid foundation on which to build more content types. Nice to get this merged - let's keep at it.

phenaproxima → created an issue.

Shipped in 0.x!

Seems reasonable but we should not be relying on the easy_email format. It's for emails.