Also, one thing we have not yet handled here, as far as I can see: What if a recipe has a dependency that is already required in the top-level composer.json?
IMHO, the existing requirement and constraint should "win", and be kept as-is. This will definitely need functional test coverage.
I ended up removing the unit test here because:
- It makes the architecture leaky; classes that should be final have to be left open so we can mock them; and methods that should be private have to be made public so we can call them.
- I'm not sure it really gives us any more confidence in the code than the functional test does. Unit tests can have an unfortunate tendency to simply become a repeat of the main business logic, and I think that's the case here. The functional test is what really shows us that the plugin works as intended, so I think we should double down on it. I think the functional test is also now testing most of what we care about, since the scope of the plugin is reduced to just handling recipes, so I'm not sure the unit test is adding much value in light of that.
This disables most(?) of the test coverage without explanation, which I don't think is acceptable. This means we might well be committing a completely broken module, which is broken for reasons we don't understand. If we don't fix at least most of these problems now, nobody will in the future.
I see one extremely strange thing in the services.yml file, which might explain some of the test failures?
@thejimbirch and I discussed this issue with @alexpott.
One thing we agreed on is that this should be for recipes only. We can remove UnpackerInterface and UnpackerFactory; they won't be needed. To make it even clearer that this plugin is only intended for use with recipes, we should rename it to drupal/core-recipe-unpack, under the Drupal\Composer\Plugin\RecipeUnpack namespace. Any configuration flags should be under extra.drupal-recipe-unpack in composer.json.
I think we also agreed that a reasonable default workflow is that the recipes are unpacked as soon as they are required into your site, and are not kept as Composer-managed packages.
Will that leave orphaned dependencies in your composer.json if you ultimately don't apply the recipe you just required? Quite possibly, yes. Having an additional plugin to remove dependencies that you're not actually using would be helpful, but is not in scope here; we could add that in a separate issue. Or maybe it could be implemented by Package Manager. Either way, it's not our problem right now.
I think that this "auto-unpack" behavior should be configurable, and enabled by default. If a site builder chooses to disable it, they can still call composer drupal:recipe-unpack foo/bar at the command line to unpack it later.
So, kicking this back for those changes.
I would suggest that we simply add this to the Update Status module as an event subscriber, respecting its opt-in/out.
@tedbow and I had a nice, long Zoom chat about #21 and the implications of it.
It is in the very nature of Package Manager to change the running code base. Right now, that only happens during the apply phase, and during that phase, Automatic Updates kicks the site into maintenance mode and tries to finish the current request as quickly as possible. This is precisely to mitigate the possibility of weird things happening while the running code base is changing itself.
The crux of the problem is that, conceptually, direct-write moves that risk from the apply phase, to the require phase. In direct-write mode, the apply phase effectively doesn't exist.
Ted and I felt that the right course of action, then, is for Package Manager to kick the site into maintenance mode when direct-write is happening. In other words -- when something is required in direct-write mode, go into maintenance mode, and emerge from it when the require has completed. The idea is to reduce the risk of weird things happening if the running code base is changed while, say, 1000 people are using the site. It can't completely eliminate the risk, but it would greatly reduce it.
Additionally, direct-write mode should result in a warning being flagged during status checks. That warning should state that direct-write mode can be risky because it changes the running code base. That is always true, and site owners should be aware of it. Direct-write mode is meant to make local development easier (i.e., a site with only one person using it), and it's inherently riskier in a production environment. Flagging a warning seems like a completely reasonable thing to do.
We think we could implement that change in a separate issue, after this one (which adds the direct-write API) is committed. That issue would be a stable blocker for Package Manager.
The second tricky thing that came up is the validators which are trying to compare the live site with the sandbox, by subscribing to PreApplyEvent. Most of these validators are just useless in direct-write mode, and their validation -- which may be valuable -- is just skipped. (For example, SupportedReleaseValidator checks to ensure you didn't just install some insecure or unsupported project into the sandbox.) How can we ensure that this validation is not lost? Even if the validation has already happened to your site, you as the site owner should still be aware of it as a problem that you need to address.
We're still not entirely sure what to do here, but Ted's idea is to shift a lot of that validation to StatusCheckEvent. Since there's no sandbox to compare the live site against, we'd have to record the state of the live site before anything else happened -- i.e., on or before PreCreateEvent is fired. Subscribers to StatusCheckEvent could then compare that recorded state to the current state of the site, to determine if anything untoward had happened during the require phase, regardless of whether it happened in a sandbox, or in the live site.
We could probably also do that in a stable-blocking follow-up issue.
Merged into 1.x and cherry-picked to 1.1.x. :)
OK, I refactored the test a bit so that the exceptions are actually caught.
I originally had it so that all entity keys are not allowed by the action, but that's going to be too restrictive, it turns out, since the label can also be an entity key, and there's no reason we should forbid something like that from changing. So, for now, only the ID and UUID keys are treated as immutable.
I'm actually not quite sure this is correct, because:
- I could be wrong about this, but I don't think those additional expectException() calls are being reached in the test. Usually the test method will exit after the first exception. So this actually needs a new test method.
- We shouldn't just handle IDs and UUIDs, we should be preventing setProperties() from changing any of the entity's keys.
- Not all entity types use 'id' and 'uuid' as their ID and UUID keys, respectively. We need to get that from the entity type metadata.
I'll deal with this tomorrow, since it shouldn't be too hard to adjust what we have. Leaving assigned to myself for that reason.
@tedbow and I discussed this at DrupalCon. We agreed that we should just not dispatch PreApplyEvent and PostApplyEvent at all, since they don't make any sense if you have effectively already "applied" changes to the site by requiring some dependencies.
So we'll just document that, and test that those events are never dispatched in direct-write. But generally this is ready for review.
tim.plunkett → credited phenaproxima → .
phenaproxima → made their first commit to this issue’s fork.
phenaproxima → created an issue.
Alright, I wrote some tests and updated the issue summary.
OK, that's a lot more elegant.
So effectively, the new setEntityProperties method will have exactly the same syntax as simpleConfigUpdate. Super clear, super simple conversion path.
I think we probably need to move this to Recipe Installer Kit, since that's where all the installer's functional code exists.
We'll also need to research where, exactly, the translations are loaded from.
The launcher script was removed from Drupal CMS 1.1.0 for exactly reasons like this (compatibility and maintenance difficulties). The 1.0.x branch is no longer supported, so this is unfortunately will not be fixed.
This seems really good and straightforward. A few relatively minor suggestions.
kristen pol → credited phenaproxima → .
kristen pol → credited phenaproxima → .
Maybe add them, if in doubt; they can always be removed in subsequent releases if they're not ultimately needed.
Another is in the config sync screen.
Not a hill I'd die on, but IMHO this doesn't add up to "put Diff into the Admin UI recipe". Managing configuration exports and syncing is generally more of a developer-ish task, so I'd put that in the Dev Tools recipe, should we ever create it (see ✨ Create a developer tools recipe for some advanced UI functionality Active ).
Removing tags; I don't think those are really used for anything.
Why would we add this to Admin UI, rather than content_type_base?
kristen pol → credited phenaproxima → .
kristen pol → credited phenaproxima → .
kristen pol → credited phenaproxima → .
This sounds like it's more of a Dashboard issue than something for Drupal CMS specifically to deal with. Tentatively closing as a duplicate.
kristen pol → credited phenaproxima → .
I don't think this exactly true if you minimum-stability: beta you have an existing page that is at 1.0.0-beta1 you should not be able to update to 2.0.0-alpha1 because even though it is an update it is going from below at minimum-stability to below it
Huh? Composer won't let you download alpha1 if your minimum-stability is beta, because that's how minimum-stability, like, works...am I missing something?
This would be relatively straightforward to do. We can build it automatically as a CI job.
- Let's call it
drupal/cms-cpanel. That should make it clear that this variant of the project template is intended for cPanel and its relatives (like Softaculous). - It will be identical to the regular project template except that
web/is./, same as drupal/legacy-project. - It should be pushed to a GitHub repository under the DA's
drupalnamespace, and published to Packagist automatically. I don't think this deserves to be a general project on d.o, that will just be confusing.
Crediting @alexpott for finding this one and reporting it to me in Slack.
phenaproxima → created an issue.
🤔 What's weird about that, to me, is that Package Manager does composer update --with-all-dependencies, which should, I'd think, get around that: https://git.drupalcode.org/project/automatic_updates/-/blob/3.0.x/packag...
But I guess a reasonable workaround would be to run composer update drupal/drupal_cms_starter at the command line first, before trying to update the theme in the UI.
To ease things somewhat, I have marked the 1.0.x version of the theme as supported, but not recommended. So at least folks won't get any more warnings in the UI about having unsupported extensions installed.
Released https://www.drupal.org/project/drupal_cms_starter/releases/1.0.4 → with this fix. That should allow you to update Drupal CMS Olivero! Thanks for catching this.
phenaproxima → created an issue.
Merged into 1.1.x and cherry-picked to 1.0.x. The 1.x branch is already using the ^ operator so no change was needed there. I will release drupal_cms_starter 1.0.4 with this change!
Duplicate of 💬 drupal_cms_starter 1.0.3 locks Olivero theme to <1.1 Active ! 😂 Closing this one and crediting you in there.
Yeah, this will get into 1.0.x as a special exception, since without that there's almost no point to the fix!
phenaproxima → created an issue.
Merged into 1.x, and cherry-picked to 11.1.x. Thanks!
Don't fork anything! :)
Just create a new general project → -- and it must be a general project, not a module or theme -- and start creating a recipe in it. You can use any of Drupal CMS's individual recipes as an example to follow, if you want. (Example: https://www.drupal.org/project/drupal_cms_content_type_base → )
Some more docs: https://www.drupal.org/docs/extending-drupal/drupal-recipes →
I agree with Pam -- I think a decoupled recipe would be very cool and useful, but it's not something we need to maintain or gatekeep. If we wanted to ship it as a core part of Drupal CMS, we could always require it as a third-party dependency (something we already do with Easy Email Express → ).
The one thing I would humbly request is that, if you create and publish a decoupled recipe (and I hope you do!), you don't publish it under the name drupal_cms_WHATEVER</em>, as that can potentially cause housekeeping headaches for us later on. It's something that other recipe authors have done without consulting us, and although it's not the end of the world, I'd prefer to keep the <code>drupal_cms_ prefix reserved for packages that are actually part of our subtree split. :)
OK - I released Drupal CMS 1.1.0 and therefore this is done!
poker10 → credited phenaproxima → .
Figure out if we can update it now, or need to wait until 1.1.0 ships.
You can update it now. There is nothing blocking in 1.1.0.
Figure out what it needs to say now.
This is the proper sequence of commands (from https://ddev.readthedocs.io/en/stable/users/quickstart/#drupal-drupal-cms):
mkdir my-drupal-site && cd my-drupal-site
ddev config --project-type=drupal11 --docroot=web
ddev start
ddev composer create drupal/cms
ddev launch
Or, if you've got the zip file unzipped (in the unzipped directory):
ddev config --project-type=drupal11 --docroot=web
ddev start
ddev composer install
ddev launch
phenaproxima → created an issue.
phenaproxima → created an issue.
phenaproxima → created an issue.
phenaproxima → created an issue.
phenaproxima → created an issue.
Took a stab at implementing #19. Still needs test coverage, a change record, and updates to the recipe system documentation.
phenaproxima → changed the visibility of the branch 3439713-should-drupalcoreconfigactionpluginconfigactionsimpleconfigupdateapply-error to hidden.
Moving this to core.
I don't think we can just flat-out throw an exception here. I think we need to deprecate the use of simpleConfigUpdate on config entities.
I'm not entirely sold on the idea of extending set and setMultiple to support nested property paths, because that would be a config action-only special-casing of those methods.
But I'm open to adding a totally new setNested action which does support setting a nested property path (it's okay if it calls $entity->set() internally), and having that be the alternative to simpleConfigUpdate.
🚢
One small thing but this otherwise seems completely straightforward to me and has proper test coverage. Once that's fixed this is an easy RTBC as far as I'm concerned.
I'm hoping committers will backport this to 11.1.x, since this is a non-breaking change in an experimental subsystem.
The test failure is in a build test, which is mystifying because build tests don't touch recipe stuff AFAIK. Is HEAD broken?
Finished the change record.
Created the change record in the other issue; we'll just get it written over there.
Created the change record, it just needs details filled in. https://git.drupalcode.org/project/drupal_cms/-/wikis/Committer-Info/Cha... can be used as a template.
Created the change record, it just needs details filled in. https://git.drupalcode.org/project/drupal_cms/-/wikis/Committer-Info/Cha... can be used as a template.
Created the change record, it just needs details filled in. https://git.drupalcode.org/project/drupal_cms/-/wikis/Committer-Info/Cha... can be used as a template.
Created the change record, it just needs details filled in. https://git.drupalcode.org/project/drupal_cms/-/wikis/Committer-Info/Cha... can be used as a template.
Created the change record, it just needs details filled in. https://git.drupalcode.org/project/drupal_cms/-/wikis/Committer-Info/Cha... can be used as a template.
Created the change record, it just needs details filled in. https://git.drupalcode.org/project/drupal_cms/-/wikis/Committer-Info/Cha... can be used as a template.
This made configuration changes and therefore needs a change record.
Merged into 1.x! Good to get this bug fixed.
This is extremely out of date, sadly, and needs to be completely refactored to be brought in line with the way the Svelte code currently works, with . I think we can certainly write an automated test of this.
And just like that, all must-have and should-have issues are fixed. This meta is fulfilled!
Fixed the misnamed file that we committed in 📌 Optimize default images Active and added test coverage to prevent this from regressing again.
phenaproxima → made their first commit to this issue’s fork.
phenaproxima → created an issue.
This is a breaking change in a minor release and therefore definitely needs a change record. We also need to update the documentation pages on drupal.org that refer to the launcher script, which is a release-blocking task.
Release notes are now handled via change records.