OK, I did some refactoring here to both bring things more in line with what Lee and Wim are asking for, and make it so that only the affected inputs are the ones that get replaced or cleared out (that bit still needs test coverage).
rsync is another one where you can configure the path to it, if it's on the web server and you have permission to execute it. If you can SSH in, which rsync should give you the path, then you can do:
drush cset package_manager.settings executables.rsync /full/path/to/rsync
Fixed. :) The "Project" field in the issue metadata is where you control that.
Sure, that sounds good. We could also just repurpose this issue and move it to Google Tag's queue, so they have the context for why we're doing this.
Probably ready for an initial review.
Did you make composer.phar executable (chmod +x /path/to/composer.phar)?
Wrote an initial (functional) test, but I will still need to update the code based on Wim's outline in #23.
Re-titled and updated the issue summary to reflect the current realities.
Added the presave hook and wrote a test. 🚀
Validation can be extremely tricky for recipe inputs, because the default value has to fulfill the constraints too. Unfortunately, something like Google Tag really doesn't have a valid default value. Additionally, recipes don't support input transformation. We could validate that the tag ID doesn't contain whitespace (an empty string would still pass that constraint), but we probably can't actively strip it out.
Looks good - I changed that description to a warning message, which I think is probably the better option (more visible). I removed some of the wording (core UI standards forbid the use of "please" in user-facing text, if I remember correctly), and wrote a test.
Manually tested this with Drupal CMS 1.1.1.
I modified its project template to require the unpacker (from a local checkout) and allow it:
composer config --global repo.unpacker path /full/path/to/drupal/composer/Plugin/RecipeUnpack
composer require --no-update drupal/core-recipe-unpack:@dev
composer config allow-plugins.drupal/core-recipe-unpack true
Then I used that modified template to create a local project:
COMPOSER_MIRROR_PATH_REPOS=1 composer create-project drupal/cms unpacktest --repository='{"type":"path","url":"/full/path/to/drupal_cms/project_template"}' --stability=dev
It worked fine -- everything unpacked:
drupal/drupal_cms_content_type_base unpacked.
drupal/drupal_cms_image unpacked.
drupal/drupal_cms_page unpacked.
drupal/drupal_cms_accessibility_tools unpacked.
drupal/drupal_cms_privacy_basic unpacked.
drupal/drupal_cms_ai unpacked.
drupal/drupal_cms_google_analytics unpacked.
drupal/drupal_cms_blog unpacked.
drupal/drupal_cms_case_study unpacked.
drupal/drupal_cms_events unpacked.
drupal/drupal_cms_anti_spam unpacked.
drupal/drupal_cms_forms unpacked.
drupal/drupal_cms_news unpacked.
drupal/drupal_cms_person unpacked.
drupal/drupal_cms_project unpacked.
drupal/drupal_cms_seo_tools unpacked.
drupal/easy_email_text_format unpacked.
drupal/easy_email_types_default unpacked.
drupal/easy_email_types_core unpacked.
drupal/easy_email_standard unpacked.
drupal/easy_email_express unpacked.
drupal/drupal_cms_seo_basic unpacked.
drupal/drupal_cms_search unpacked.
drupal/drupal_cms_remote_video unpacked.
drupal/drupal_cms_authentication unpacked.
drupal/drupal_cms_admin_ui unpacked.
drupal/drupal_cms_starter unpacked.
The require section of composer.json is extensive, but contains no recipes:
"composer/installers": "^2.3",
"drupal/add_content_by_bundle": "^1.2.2",
"drupal/address": "^2",
"drupal/addtocal_augment": "^1.2.3",
"drupal/ai": "^1.0.4",
"drupal/ai_agents": "^1",
"drupal/ai_image_alt_text": "^1",
"drupal/ai_provider_anthropic": "^1",
"drupal/ai_provider_openai": "^1",
"drupal/automatic_updates": "^3.1.7",
"drupal/autosave_form": "^1.10",
"drupal/better_exposed_filters": "^7",
"drupal/bpmn_io": "^2.0.6",
"drupal/captcha": "^2.0.7",
"drupal/coffee": "^2",
"drupal/core": "^10.4 || ^11",
"drupal/core-composer-scaffold": "^11.1.5",
"drupal/core-project-message": "^11.1.5",
"drupal/core-recipe-unpack": "@dev",
"drupal/core-recommended": "^11.1.5",
"drupal/dashboard": "^2",
"drupal/drupal_cms_analytics": "^1",
"drupal/drupal_cms_olivero": "^1.1",
"drupal/easy_breadcrumb": "^2.0.9",
"drupal/easy_email": "^3.0.2",
"drupal/easy_email_theme": "^1",
"drupal/eca": "^2.1.4",
"drupal/editoria11y": "^2.2",
"drupal/field_group": "^3.6",
"drupal/focal_point": "^2.1",
"drupal/friendly_captcha_challenge": "^0.9",
"drupal/friendlycaptcha": "^1.1",
"drupal/geocoder": "^4.10",
"drupal/geofield": "^1.47",
"drupal/gin": "^4.0.6",
"drupal/google_tag": "^2.0.7",
"drupal/honeypot": "^2.1",
"drupal/klaro": "^3",
"drupal/leaflet": "^10.2.33",
"drupal/linkit": "^7",
"drupal/login_emailusername": "^3",
"drupal/mailsystem": "^4",
"drupal/menu_link_attributes": "^1.5",
"drupal/metatag": "^2",
"drupal/pathauto": "^1.13",
"drupal/project_browser": "^2-beta1",
"drupal/recipe_installer_kit": "^1-alpha3@alpha",
"drupal/redirect": "^1.10",
"drupal/robotstxt": "^1.6",
"drupal/sam": "^1.2",
"drupal/scheduler": "^2.2",
"drupal/scheduler_content_moderation_integration": "^3.0.4",
"drupal/search_api": "^1.36",
"drupal/search_api_autocomplete": "^1.9",
"drupal/search_api_exclude": "^2",
"drupal/selective_better_exposed_filters": "^3",
"drupal/seo_checklist": "^5.2.1",
"drupal/simple_search_form": "^1.5",
"drupal/simple_sitemap": "^4.2.2",
"drupal/sitemap": "^2",
"drupal/smart_date": "^4.2.1",
"drupal/svg_image": "^3.1",
"drupal/symfony_mailer_lite": "^2.0.1",
"drupal/tagify": "^1.2",
"drupal/token": "^1.15",
"drupal/token_or": "^2.2",
"drupal/trash": "^3.0.13",
"drupal/webform": "^6.3-beta1",
"drupal/yoast_seo": "^2.1",
"drush/drush": "^13",
"geocoder-php/nominatim-provider": "^5.7",
"league/commonmark": "^2.4"
(drupal_cms_analytics is a metapackage, and drupal_cms_olivero is a theme).
composer validate was happy; no errors were found.
The real test, of course, was being able to actually install Drupal CMS. I had to make a small change to Recipe Installer Kit, since it expects all recipes to be Composer-managed. (Not in scope here.) But installation succeeded, with all optional recipes selected!
This truly has been reviewed and tested by the community. Let's get it into 11.2 so Drupal CMS can rightfully adopt it and blaze the trail of update and maintenance path for sites built on a foundation of recipes.
phenaproxima → created an issue.
Manually tested the latest against Drupal CMS and ran into a failure during composer create-project:
In VersionParser.php line 526:
Could not parse version constraint (^10.3: Invalid version string "(^10.3"
Correct.
hestenet → credited phenaproxima → .
hestenet → credited phenaproxima → .
hestenet → credited phenaproxima → .
Actually, never mind - the issue summary definitely says what a reasonable test would look like.
Took a crack at this - still needs tests and such, but does it look basically like what you're hoping for? I'm not sure what to do if a prop expression in a content template is referring to a removed field, but I'm hoping that just unsetting the input will allow it to fall back to the example value.
Spun off ✨ The project templates, and Package Manager, should allow the PHP-TUF plugin Active as a Package Manager beta blocker to save us some pain later.
phenaproxima → created an issue.
Couple minor points but I'm happy with this. If the subsystem maintainer signs off, ship it. Call this a soft RTBC.
phenaproxima → created an issue.
I've reviewed this extensively and I feel pretty confident about it. I think Alex had found another bug in here that still needs to be fixed, so leaving at "needs work" for now, but this is close to RTBC. Adding at least partial credit.
Drupal CMS needs this for long-term viability, so tagging it as Drupal CMS-critical.
Took a bit of doing but I gave this a manual test with Drupal CMS.
All of this appeared at the end of the composer create-project output:
drupal/drupal_cms_starter unpacked successfully.
drupal/drupal_cms_admin_ui unpacked successfully.
drupal/drupal_cms_authentication unpacked successfully.
drupal/drupal_cms_remote_video unpacked successfully.
drupal/drupal_cms_search unpacked successfully.
drupal/drupal_cms_seo_basic unpacked successfully.
drupal/easy_email_express unpacked successfully.
drupal/easy_email_standard unpacked successfully.
drupal/easy_email_types_core unpacked successfully.
drupal/easy_email_types_default unpacked successfully.
drupal/easy_email_text_format unpacked successfully.
drupal/drupal_cms_seo_tools unpacked successfully.
drupal/drupal_cms_project unpacked successfully.
drupal/drupal_cms_person unpacked successfully.
drupal/drupal_cms_news unpacked successfully.
drupal/drupal_cms_forms unpacked successfully.
drupal/drupal_cms_anti_spam unpacked successfully.
drupal/drupal_cms_events unpacked successfully.
drupal/drupal_cms_case_study unpacked successfully.
drupal/drupal_cms_blog unpacked successfully.
drupal/drupal_cms_google_analytics unpacked successfully.
drupal/drupal_cms_ai unpacked successfully.
drupal/drupal_cms_privacy_basic unpacked successfully.
drupal/drupal_cms_accessibility_tools unpacked successfully.
drupal/drupal_cms_image unpacked successfully.
drupal/drupal_cms_content_type_base unpacked successfully.
drupal/drupal_cms_page unpacked successfully.
...and they were all in recipes:
➜ unpacktest ls recipes
drupal_cms_accessibility_tools drupal_cms_person
drupal_cms_admin_ui drupal_cms_privacy_basic
drupal_cms_ai drupal_cms_project
drupal_cms_anti_spam drupal_cms_remote_video
drupal_cms_authentication drupal_cms_search
drupal_cms_blog drupal_cms_seo_basic
drupal_cms_case_study drupal_cms_seo_tools
drupal_cms_content_type_base drupal_cms_starter
drupal_cms_events easy_email_express
drupal_cms_forms easy_email_standard
drupal_cms_google_analytics easy_email_text_format
drupal_cms_image easy_email_types_core
drupal_cms_news easy_email_types_default
drupal_cms_page README.txt
composer.json had significantly more dependencies than it normally would with Drupal CMS, including some recipes. For example, "drupal/drupal_cms_content_type_base": "~1.1.0" and "drupal/drupal_cms_image": "~1.1.0" were in require. This makes sense -- those packages are recipes, but they are strictly a dependencies of other recipes. Did we decide, ultimately, that we were not going to unpack recursively? Or is this a bug?
I was not, unfortunately, able to install Drupal CMS with all optional recipes via the UI. I ran into this:
OutOfBoundsException: Package "drupal/drupal_cms_starter" is not installed in Composer\InstalledVersions::getInstallPath() (line 252 of /Users/phen/Sites/unpacktest/vendor/composer/InstalledVersions.php).
The recipe is physically present, but this is a bit problematic -- the installer needs to be able to ask InstalledVersions where a recipe is. I think this means that we'll have to add drupal/drupal_cms_starter to the ignore list in our project template, or make the installer code fall back to some kind of dumb "guess the recipe path" logic, based on the installer-paths configured in composer.json. I don't love that, but it's not the end of the world.
Tagging for a follow-up per https://git.drupalcode.org/project/drupal/-/merge_requests/11071#note_50....
This looks great to me. There are a couple of minor gaps here, such as:
Testing that a preassigned UUID is preserved.
Testing that the action only works with config entities that implement SectionListInterface, and is not found for any other entity type (i.e., coverage of the deriver).
Since I agree that we'd really like this in 11.2, I think it is okay to add that coverage in a follow-up. Tagging for that, but otherwise I say we should ship this.
Damn it, I just realized that this update path is more complex than previously thought because Views blocks might be embedded in Layout Builder displays. (Same thing goes for 📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active but I guess that can be handled in a follow-up during beta.)
Merged in changes from 11.x.
phenaproxima → created an issue.
phenaproxima → made their first commit to this issue’s fork.
I'm a little puzzled. End-to-end tests are failing because there are several places where they assert that the form build ID has changed. That, apparently, is now inverted by what we're doing here. What's the correct approach here -- do we expect the same form build ID to persist (in which case these assertions need to be changed), or do we expect a new build of the form, populated by data from the auto-save manager?
Will start this tomorrow based on the very clear suggestions in the issue summary. Thanks @larowlan!
Added a basic set of assertions to prove that the JS component's cache metadata is added to the render array, but I'm leaving the "needs tests" tag here because we should still have coverage to prove that a nested component's cache tags are added.
phenaproxima → made their first commit to this issue’s fork.
Looks like some stuff broke. :)
Change record written: https://www.drupal.org/node/3522240 →
Moving the update path around was a very minor lift, so restoring RTBC here on the assumption that tests will pass.
Update path test written! This should still probably be committed after 📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active , but it's reviewable now.
📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active is RTBC and unlikely to need any further changes before commit, so I'll proceed with this issue and base it in what's in there.
I've run into this before, and making the property protected is the correct fix, but I'd suggest putting a comment about why that's the case, because unless you have the context (and the willingness to debug the craziness of DependencySerializationTrait), putting a protected member in a final class looks like a mistake.
Update: FieldTypeUninstallValidatorTest now passes everywhere except on MariaDB. I imagine this is not a blocker, but worth calling out.
Wrote a CR and linked to it from the deprecation message, so back to RTBC per #13.
I've removed as many json_encode() calls as I could find. I don't think we should remove the BC layer yet because when the value is pulled from the database as a JSON string (when loading the fields), the data type plugins need to be able to handle that...right?
OK, I think I mostly have tree using regular arrays now, too.
The lone exception, and the reason the BC layer that converts between strings and arrays is needed, is that field items on content entities won't be storable as JSON transparently until 📌 Allow field types to control how properties are mapped to and from storage Needs work , so until then, we probably need to keep supporting strings of JSON. But we definitely don't seem to need those for storing trees in config.
I don't think we need to add a whole bespoke function for this; wouldn't \Symfony\Component\Filesystem\Path::isBasePath() do the trick for us? Agreed that extra test cases couldn't hurt.
The patch should be against core, not Automatic Updates.
EDIT: I see the bug here -- the str_contains() is not at all erroneous, but it's finding that both the active and stage start with the prefix /var/www!
If you rename the temp directory to something totally different, like /var/drupaltemp or something, and adjust your settings.php to match...I bet that error will go away.
Maybe, but that's a separate issue that's not in scope here. :)
Thanks for looking at that! I think this is clearly a bug and needs further investigation.
Okay, I made inputs a standard array -- does this look more or less correct? (I know that ignore is the wrong config schema type, but the tree and its inputs are validated as a whole, so I'm not sure it needs to be anything more specific).
I think that, to not break everything everywhere all at once, we might want to do this in multiple issues (first deal with inputs, then tree in another issue).
And I'm also thinking that, until 📌 Allow field types to control how properties are mapped to and from storage Needs work is done, we might want to have the ComponentTreeStructure and ComponentInputs data types handle both arrays and JSON blobs. That would ease the difficulty of the transition without blocking us, it looks like.
I asked @effulgentsia if there was any particular reason why the XB trees are stored as JSON blobs. His answer on Slack:
I'm not aware of any good reason to keep them as JSON blobs. I think the only reason we didn't embed them as a proper structure within the config to begin with is just at that time not wanting to deal with config schema and validation. But now is the time to deal with that. In that issue.
I don't think this will affect config actions much, either way. They are severely limited in their ability to manipulate complex arrays, and regardless of how the data is stored at rest, we'll need a slew of custom config actions to make XB useful to recipes.
But it'd certainly improve DX to have the tree be a YAML structure. I personally don't see any reason why it needs to be a JSON blob, since YAML is a superset of JSON. I don't understand why it would be hard to move back and forth between JSON and YAML if we wanted to -- if you can decode some YAML to a PHP array, you can re-encode it as JSON.
Blocking this on 📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active , which makes a change to the BlockSettings migration plugin that will benefit this issue. (The change is complicated enough that it requires several tests to be adjusted, so I'd rather postpone than cross-port.)
Update path is written, but still needs a test.
phenaproxima → made their first commit to this issue’s fork.
And, added an update path test. I think this is reviewable now.
Tests written. Just need to test that ol' update path!
The point here is Drupal CMS is supposed to be a curated set of modules which are expected to work together without causing WSODs.
This is true, but we can't account for every possible combination of every possible module that other modules depend on (or include as submodules). We have no control over that.
What we do have control over, and what we do stand behind, is that the modules we install as part of our recipes -- "install" meaning it's actually activated in Drupal, rather just sitting inert in the codebase -- should work together.
So if we are not enabling Search API DB Defaults, despite it being shipped with Search API (which, again, we cannot control) -- and there's a bug in it, that would make it squarely a bug in Search API and needs to be fixed there.
Thanks for this clear, detailed bug report -- I'm ensuring you get issue/commit credit.
Wow, that's a weird one. I've never seen this before. What kind of database are you using?
Can this wait until Drupal 11.2 is out? The reason I ask is that core changed the extension system so that a batch install like Drupal CMS's should have far fewer container rebuilds, which should result in a significant performance improvement. It won't get down to seconds or anything like that, but it might be a big enough dent that we'll want to reevaluate what to do during this wait.
Wrote the update path. But it does need an explicit validation test.
Wouldn't it be better to make depth optional, aka nullable: true? Then we don't need to assign a special meaning to 0
Not sure we should do that, as it would constitute an API change for...what benefit, exactly? Maybe a separate issue would be better here.
$this->menuTree->maxDepth()
Fun fact: \Drupal\Core\Menu\MenuLinkTree::maxDepth() calls \Drupal\Core\Menu\MenuTreeStorage::maxDepth(), which does no computation at all, but merely returns...a constant (\Drupal\Core\Menu\MenuTreeStorage::MAX_DEPTH).
That constant is older than two of my nieces. It was created in #2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage → and appears to have been completely unchanged since.
So, to get this issue done faster, I propose that we don't even need to worry about having a special constraint to determine the max depth dynamically. Let's just hard-code it, at least for now, based on the value of that constant, which has not been changed in core since it was introduced.
That way, we'll get pretty strong validation done quickly, without a ton of effort, and we could make it a more flexible constraint later in a follow-up.
I did not expect this issue to be so dragon-shaped.
I discussed the problem with @larowlan. The conundrum is that you (currently) kinda have two ways for an XB field to exist (obviously this could be subject to change, but this is the idea being implemented in this MR thus far):
- On the xb_page content entity type, which is a single-value field that contains an entire tree which is used for rendering.
- On any other content entity type, which is an infinite-cardinality field where each item is a bonsai tree that is injected into an exposed slot in a content template
In the first situation, "which slot is this tree in?" doesn't matter. The tree is self-contained and complete.
But in the second situation, "which slot" is critical! The subtree must must be associated with a specific slot. Since the slot is defined and exposed by the content template, it is inextricably linked with that at the data layer.
@larowlan feels (as @longwave alluded to in his review) that this boils down to being, effectively, two different field types -- the only differences between them are:
- the usage of a
slotproperty - the enforced cardinality (either 1, or infinite)
Forgive my idiomatic metaphor here, but this is a conversation that needs to be had amongst the grand poobahs of Experience Builder (Lauri, Wim, etc.) and the decision probably needs to be documented with an ADR. It is very out of scope for this issue.
Exacerbating the problem is that the frontend has absolutely no concept of "this tree will fit into a slot". The backend doesn't tell the frontend what slot in a template we're working on, and the frontend therefore doesn't tell the backend. That's what's breaking the tests right now in this MR.
So...where does that leave this issue? Lee felt that we should do this -- in order:
- Greatly reduce the scope of this issue to only add the
ComponentTreeItem::mergeSubTrees()method and its test coverage. We're gonna need that in any event, since that's key to combining a template's tree with bonsai trees stored elsewhere. - Make the frontend recognize the concept of "I'm working on a particular slot". That might not necessitate any user-facing change, but the backend needs to transmit the name of a slot (or NULL) to the frontend, and the frontend needs to transmit that information to the backend.
- Make an architectural decision about how Drupal will store trees, regardless of whether the target slot is relevant. Two different field types? Some other kind of magic? Removing the xb_page entity type and allowing nodes to contain complete trees (something Lee floated in our call)?
Whew.
OK, so the question is...does this still persist even with the post-update function running properly?
Merged into 2.0.x.
Yeah, let's deal with those later. A lot of those are just because there's not much point in committing useless doc comments comments like "This class implements SomeInterface" (since you can see that by reading the code). It's better to leave a few coding standards violations in place and fix them usefully later in a separate issue, when we still were able to fix a lot of other coding standards issues in this one. 👍
Adjusting credit, since you've been helping me test this out.
You know...I just realized that I mis-named the hook. Let me push a correction there...
It's a post-update hook. They're run as part of normal database updates (update.php, or drush updb). The _for_v2 suffix is just the arbitrary name assigned to the hook, that's how they work: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
Hmmm...well, the post-update has to run first, before the container is rebuilt. Did you do drush updb first?
phenaproxima → made their first commit to this issue’s fork.
Thank you!!
It got us past the container problem, though, didn't it! So that's progress.
Next step is to open an issue to add an empty post-update hook to force a container rebuild. Let's deal with this new problem separately, in yet another issue. I really don't want to just keep throwing problems at one mega-issue. Small, tightly scoped issues we can quickly iterate on, that's the ticket.
That's why I'm not suggesting that. Quoting #38:
An empty post-update function should cause a container rebuild, so we should probably just add one -- in a new issue -- to deal with that.
I'm just waiting for you to confirm that clearing the cache tables (so as to force a container rebuild) does the trick. :)
A refreshingly easy test to update. Those are a bit hard to find in XB 🙈
Well, that's annoying. In that case, I guess I'd try going into the database directly and truncating all of the cache_* tables.
phenaproxima → made their first commit to this issue’s fork.
Normally drush cr will do the trick.
How do I reproduce #35 on a clean Drupal 11 install with Layout Builder ST 2.0.x? I'm not seeing it. I don't think this will happen on new installs, only existing ones that go directly from 8.x-1.x to 2.0.x, because the service definition changed.
An empty post-update function should cause a container rebuild, so we should probably just add one -- in a new issue -- to deal with that.
The failures are very tricky to solve. They are due to @larowlan's request for us to validate that component trees attached to most entities have a slot name. That is NOT sent from the client side, currently, or handled by the ClientDataToEntityConverter service, so tests break and I'm not sure how to fix them.
longwave → credited phenaproxima → .