Fixed in 2.3.1
@joelsteidl: It looks like class_exists(AbirtaryUndefinedClass::class) will return false without throwing an exception. This fix exists in the dev branch right now; I will be releasing a 2.3.1 release shortly.
I fixed via https://github.com/pantheon-systems/pantheon_advanced_page_cache/pull/60..., but I am now wondering if I need to use a string constant as @trackleft2 suggested, as ImageStyle::class is likely also to produce a class not found error.
Presumed to be fixed by 2.3.0
Fixed in 2.3.0
I tried deleting node.field_image and user.user_picture (perhaps you do not get these if your site is installed from the Minimal installation profile), and disabled the Image module. I still could not reproduce.
Perhaps you are using some other module that creates files with a mime type of 'image' without using the image module. I added a speculative fix based on the symptoms to the [Drupal 11 support work](https://github.com/pantheon-systems/pantheon_advanced_page_cache/pull/58). I would have split this out into its own PR, had I been able to reproduce it.
I cannot reproduce.
From a fresh install of Drupal 10, I create a new node of type "Article" and follow the instructions in the problem description above. The page saves correctly with no WSOD.
From the symptoms, it looks like the "Image" module is disabled in your site. Could you confirm that this module is enabled? This module is required by node.field_image and user.user_picture, but it is conceivable that you may have disabled these. As a workaround, try enabling the Image module.
Well, Greg expressed his opinion in the referenced Drupal Slack about why he would not rely on core-dev for running tests on downstream projects
Don't worry about that old comment, it was unfounded. My point is that you shouldn't use core-dev unless you're using test classes packaged by Drupal to run tests on downstream projects, which is exactly what folks are doing.
My latest comments on moving forward, copied from slack:
It needs to be tested. Testing wouldn't be super easy; someone needs to clone the various Drupal projects with all / most of the versions tagged, and then make some test Drupal sites that are out of date, and try to run "composer update" on them. The proposed dependencies were too much for Composer some years ago; the hypothesis is that modern Composer will handle it just fine. This is likely the case, but it would be unfortunate to deploy the changes to everyone & then find out in the field that it doesn't work well.
To do a good test, you'd probably want to rewrite the tags so that the old versions in the repo have the new dependency relationships. If you only made the proposed changes for the newest tag (as would happen when the MR is merged), then it's possible that Composer would do okay if there was only one version with the new dependency relationships, but might get confused in the future when they started to grow in number.
If I recall correctly, the conflict statement that I recommended worked better (e.g. #9 can be rewritten to allow only one patch version). I don't recall if there was any reason we didn't add it; maybe just never got to it. We certainly did not want to encourage folks to mix different core versions.
In what way?
We now recommend that upgrading core be done with "composer update drupal/core*"
I believe I answered this question elsewhere recently, but I am not sure where, so I will re-post here for context.
During the development of Drupal 8.8.x, composer update drupal/core* failed to update, because Composer became confused by the chained strong constraints. One might think that strong constraints would make the dependency graph easier to resolve, because it branches to fewer variations, but for whatever reason, it was harder for Composer in the past. If modern Composer deals with it well, then this change is an improvement.
I like the discovery option proposed in #47. In the issue linked there, there is also a proposal to expose core/scripts/drupal as vendor/bin/drupal. No one from Drupal Console has weighed in on any future development plans related to that issue yet.
I will also point out that Starshot ships with Drush pre-installed, which will be helpful for folks who start with that as a base, rather than Drupal core. For core development, though, I like #57.
Being tracked internally as CMSP-722, which also covers PHP 8.3 compatibility.
I can't help but feel like some of the strong opinions against combining dex and drupal might be motivated by an impression that taking that step would pull in a lot of complexity from the Drush bootstrap logic in order to support both bootstrapped and non-bootstrapped commands. In actuality, though, this can be achieved with a little over a dozen lines of code by overriding the Application::find() method. Drush has been doing this successfully for years, since Drush 9, and it has worked well across multiple versions of Symfony. The rest of Drush's preflight and bootstrap can be left behind. The key part look like this (in the derived Application class):
public function find($name): Command {
try {
$command = parent::find($name);
if ($command instanceof ListCommand || $command instanceof HelpCommand) {
$this->bootstrap();
}
return $command;
} catch (CommandNotFoundException $e) {
if (!$this->bootstrap()) {
throw $e;
}
return parent::find($name);
}
}
I have created a branch that demonstrates the above technique in action. Bootstrapped and non-bootstrapped commands live side-by-side in the drupal script. This fulfills the vision from the IS: to eventually replace dex with a unified drupal command. The rest of the code in the branch needed to make this work, beyond the fourteen lines shown above, is pretty much the same code that is in the current MR, with some refactoring. I have left the original dex script in the branch, so that it can be tried alongside the enhanced drupal script. Some enhancements from dex, e.g. the wrapper in vendor/bin, have been carried over from the original MR and applied to the drupal script.
I think that the work done in the original MR here is really excellent. The advent of attribute-based discovery, and conformance to the latest Symfony conventions around command definition gives us a really clean interface to allow Drupal modules to add cli commands to a lightweight front controller in core. I think that we have a really good chance at getting this merged, if the community can reach an agreement on whether the script that is used to call Drupal module commands should be called dex or drupal. I hope that the minimum additions I've shown here are sufficient to convince folks that we should integrate with the existing drupal script now rather than later.
The issue summary says:
Perhaps a future all-encompassing Drush replacement would utilise the drupal namespace and cleanly replace dex.
It is not necessary to build an entire Drush replacement to unify the dex and drupal front controllers. We have a paved path for doing this in Drush that wouldn't be too difficult to use here. It's harder to remove things that you don't want than it is to add something new, so I do not find it to be a compelling argument that we should introduce a new script only to immediately deprecate it.
I think we could get this merged if we unified on the drupal script name. I personally feel like we have too many different command names that are trying to do the same thing, and expanding that collection further is not moving in the right direction.
The main use-case for non-bootstrapped commands is the existing drupal install.
Overall, I like the proposal. Drush can already accept Symfony Console commands in its command list, so if the command loader was available as a public method of a discoverable class, then all dex-provided commands from modules could also be available via drush commandname, for existing Drush users, as desired.
Regarding the Drupal core cli, though, I think that the command name should remain drupal, and the work here should be limited to providing a mechanism whereby modules can add new commands to this existing script.
@alexpott: you are correct, the summary is out of date. It should say ANSI and TRADITIONAL rather than listing all of the individual modes that Drupal used to set. If we did https://www.drupal.org/project/drupal/issues/3261236 → , then Drupal would use individual mode values again. I don't know if that issue is a good idea or not, but if it is, it wold be better to do this one first.
There is very little motivation for individual users to set these mode values in most cases. Right now, Pantheon is tracking an issue were sites migrated from other systems where they used to run on Mysql sometimes suffer performance penalties when the same SQL queries run on Mariadb. Allowing control over individual modes would give ISPs more flexibility, e.g. we could set a mode for a group of sites, and individual sites could later alter individual modes further if necessary.
Another concern I have is that providing an API for this means that we have to track changes and do deprecations for values when the underlying modes change in the database API.
That's fair. Maybe it's better to go back to strings of values instead of providing a class with constants.
I think we also need testing for the different combinations we can set with sql modes. Specially for edge cases.
I added SqlModeSelectionTest, which tests every condition and edge case I could think of related to selecting or de-selecting Sql modes. I think all of the review comments have been responded to.
Updated Sql Mode docs to include a reference from default.settings.php per suggestion by @chx. (https://drupal.slack.com/archives/C1BMUQ9U6/p1717457117657049?thread_ts=...)
Ok, "needs review queue bot", come at me. I actually expect it is going to "needs work" me, because the "MR" badge is grey instead of green, even though the branch is up-to-date with HEAD of 11.x. Setting back to "needs review" for the record, though.
Updated the change record → to reflect current targeted Drupal core versions.
Hum. Seems to still be up-to-date to me. :shrug:
Thanks, Needs Review Queue Bot! Rebased to HEAD of 11.x branch.
Switched this to deprecate sql_mode in 11.1.0 for removal in 12.x, and backported sql_mode_options to the 10.4.x branch without deprecation of sql_mode.
greg.1.anderson → changed the visibility of the branch 2939760-10.3-allow-granular-overriding-of-sqlmode-options to hidden.
Looking into test failures.
Regarding the last change I pushed, while it would be possible to simplify the logic slightly, I wanted to point out that I deliberately ordered things the way I did to minimize the number of lines that had to be touched when the deprecated sql_mode support is removed.
Updated the MR (11.x branch only) to deprecate the feature in 11.x, and remove in 12.x. Question: for the deprecation message, should it be version 11.1.0, or is 11.0.0 ok?
Thanks!
Yeah, #81 sounds reasonable to me.
I added a good test that demonstrates that we are able to change SQL modes. Two tests are done:
1. There is a test that checks the SQL mode and fails if the new mode added in the setup function is not listed.
2. There is a test that confirms the behavior of ONLY_FULL_GROUP_BY. This test will fail if that mode is not correctly applied in the setup function. This is essentially the test suggested in
#3261236-3 →
. (It differs in that we explicitly set ONLY_FULL_GROUP_BY in the test; we could/should copy this test to core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest when updating #3261236, to ensure that default behavior includes ONLY_FULL_GROUP_BY for both Mysql and Mariadb).
An existing test confirms that the default SQL modes are not set when removed in the setup function.
Remaining questions:
- Are there more edge cases we should be testing? I can not think of any that are needed, but if anyone can think of something that should be covered, let me know and I will add more tests as needed.
- Do we need to worry about making it easy to remove Drupal default options? I am inclined to go with option 2 from #78, above ✨ allow granular overriding of sql_mode options Needs work
- Right now, we are deprecating SQL mode in 10.3.x, and removing it in 11.x. Do we need to deprecate it in 10.3.x and 11.x, for removal in 12.x?
I think that the current MR satisfies all of the remaining questions, at least for the resolution of each that I suspect. If there is general agreement on that, then we're ready to go back to RTBC.
I realize that I was not correct above; sql_mode_options is merged with the default options, so there is no way to blanket-reset the Drupal defaults. In order to eliminate a setting that Drupal applies, you must explicitly set the default modes to FALSE. This leads us to two alternatives:
1. We could un-deprecate setting sql_mode in init_commands, as this setting provides a capability that sql_mode_options does not.
2. We could address this edge case in #3261236, and simply not add any modes related to ANSI or TRADITIONAL if sql_mode_options sets ANSI or TRADITIONAL to FALSE, respectively.
@darvanen yes, please, as @joachim mentioned, please review both the 11.x and the 10.3.x branches. The change record is for the 10.3.x branch. Do we need a second change record for the 11.x branch, describing the removal of the deprecation checks?
I have reviewed and tested this MR per #67 and #65, and believe this is now ready for review by a core committer. I did make a couple of commits; a one-line change (#56), to make the tests pass, and added some documentation to default.settings.php. Hopefully this involvement is minor enough that it is okay for me to still RTBC here.
Regarding #17, the first and third requests have been done. As for testing, the existing functional tests wouldn't pass if the logic here was faulty, so I feel like existing coverage is adequate. If there's anything else desired here, though, please let me know.
I did the same test as #65 on a Drupal 11.x-dev site, after adding the patch, and got consistent results there as well.
Update release notes snippet.
I upgraded a site to 10.3.x-dev and applied the patch to it. After updating setting.php to include:
$databases['default']['default']['sql_mode_options']['ONLY_FULL_GROUP_BY'] = TRUE;
Running drush ev '$database = \Drupal::database(); $query = $database->query("SELECT @@SESSION.sql_mode"); $result = $query->fetchAll(); var_export($result);' before and after applying the settings.php change showed that the mode ONLY_FULL_GROUP_BY was, in fact, added to the SQL Mode after adding that modification.
Ironically, the failing test that was confusing me was one that I wrote during the composer initiative, to ensure that no one ever forgot to change both the scaffold assets and the original file, which of course I did. Not sure why the failure wasn't visible until I looked at the raw output, but once I did that, the fix was clear, and the 11.x branch is green again. I just pushed the same fix to the 10.3.x branch, which is now running.
Changed it for good measure (and correctness of the comment); if nothing else, this will give the testbot another chance to run it again.
I added a comment-only change, and the phpunit tests are now marked as failed with exit code 1, even though there are no error lines in the job output. Not sure if a warning is triggering the bad exit code, or if it's just a job runner failure.
Rebasing fixed the 10.3.x branch.
Now I am seeing the same result as #53 on the 10.3.x branch. Didn't see that at all on the 11.x branch. Bringing us up to date with HEAD of 10.3.x, to see if that helps.
My hypothesis seems to have held out; the 11.x branch tests are now green. I have pushed the same fix for the tests to the 10.3.x branch, which is running now. There isn't any need to backport this to 10.1.x or 9.x, is there? Shall we simply hide those branches?
Regarding the test failures, we have this in the latest 11.x run:
1)
Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryMultilingualTest::testConfigSync
Behat\Mink\Exception\ElementNotFoundException: Form field with
id|name|label|value
"Drupal\mysql\Driver\Database\mysql[sql_mode_options][ANSI]" not found.
/builds/issue/drupal-2939760/vendor/behat/mink/src/WebAssert.php:731
/builds/issue/drupal-2939760/core/tests/Drupal/Tests/UiHelperTrait.php:85
/builds/issue/drupal-2939760/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php:265
/builds/issue/drupal-2939760/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php:174
/builds/issue/drupal-2939760/vendor/phpunit/phpunit/src/Framework/TestResult.php:729I haven't looked at the code from the stack trace yet, but I'll see if I can figure out what is going on. Maybe it's only in the tests, or maybe it's in the config code, but it looks like something might be building forms based on the values in the $databases array, which is to say, this might be related to moving sql_mode_options to the top level of the database info array.
Do you mean whether we should totally ignore it, or throw a warning if it's present and then unset it?
Yes, I think it should be as you did it in the latest pair of MRs: 10.3.x should warn, but still respect $connection_options['init_commands']['sql_mode'], and 11.x should silently unset it.
Fix typo in issue summary ("TRADITIONAL" instead of "NO_ENGINE_SUBSTITUTION" in Mariadb list of modes implied by TRADITIONAL). Add bolding for differing modes.
Update summary to show that sql_mode_options is a top-level element of the $databases array.
A question about the implementation. This MR adds a new field, 'sql_mode_options', to the 'init_commands' portion of the $databases array. However, 'init_commands' is defined to be a list of SQL expressions to execute, leading to a bit of awkwardness where we need to un-set 'sql_mode_options' before executing everything else in the 'init_commands' list. On the surface, it seems better to make 'sql_mode_options' a top-level element of the $databases array, since it is unlike everything else inside 'init_commands'. Is there any reason why it would be inconvenient or infeasible to add a new top-level element to the $databases array?
This issue overlaps with #2939760
Patch looks good to me -- thanks for finding this and submitting a test. It definitely should have been like this from the beginning.
I would be on-board with ensuring that the Drush 8 pm-* commands work with the new location, once it is available. This should already be possible to do this just with configuration, but we could make it work out of the box with a new Drush 8 release.
At the time of this writing, Pantheon does not support injecting the new database settings introduced in this change record into the $databases array. Note that the two drivers that need this feature today are not necessary on Pantheon. Tracking internally as CMSP-783.
Excerpt from the internal ticket:
Possible things we could do:
- The change record shows how to get the information that the installer injects into the $databases array. We could use an internal job to query this information, and then write it back into our internal storage so that we can correctly populate the $databases record. Note that the difficulty here is that we probably can’t make these calls until after Drupal has been installed, and also, the info injected by the installer could change in future Drupal releases, making our code out of date.
- We could look for other methods we might call that are already in use by the installer that provides all of the information that should go in the $databases record.
- We could collaborate with the Drupal community and try to determine ways that Pantheon could signal the installer to call back to Pantheon with the info it would like to write into the $databases record, so that we can store it in C*.
Regarding #9 above, the new Command mechanisms from Drush 12 were backported to the most recent versions of Drush 11, so you can go ahead and adopt the new recommended static Create method, and your code will still be able to work on Drupal 9.
greg.1.anderson → created an issue.
markdorison → credited greg.1.anderson → .
Drush 8.4.11 was released Jun 2, 2022; I will be making another release shortly. Note, however, that Drush 8 no longer works with Drupal 9, and will not work with Drupal 10, so it would be fine to add a conflict on Drush 8 in Drupal 10.
The existing "drupal" script does not bootstrap the kernel. The second example in #190 looks incorrect to me, but I did not try to install Drupal using this patch. Maybe someone else can explain why the wrapper script is like this.
#188: It would not be possible to write commands like "site install" if a full bootstrap was always necessary. It is also much faster to omit the bootstrap for commands that do not need to call the full Drupal API suite. That said, I do not know how many of the commands being considered for inclusion in core will run with a partial bootstrap.
We can't inject the service provider because the kernel doesn't give us any opportunity to.
But we are core. Can we add a setter to the Kernel?
The magic name is the name of the class we discover.
How would a module add a command with this mechanism? It looks like all of the command names (just one ATM) are listed in code. This is fine for core commands, but we need discovery for modules. Maybe I'm just not seeing it.
I do not dispute that it is possible to use composer.json for such purposes; there are use cases where it is reasonable to put non-Composer metadata in composer.json. However, it's not the best idea. I am skeptical about
✨
Replace .info.yml with composer.json for extensions
Postponed
, and we probably would have put the Drush metadata in the info.yml file instead of the composer.json file but for the fact that composer.json has extra and info.yml files do not.
The larger issue with #157 is not that it reads info from composer.json as much as that it has to search the filesystem for composer.json files and then aggregate information from them. Discovery should happen using information already available to use. There's an example above that does it, but I'll have to look at it later.
As another example, Robo's plugin mechanism uses $this->classLoader->getPrefixesPsr4() as $baseNamespace => $directories to find the set of namespaces to search, and then uses a filesystem search over just a small subset of the filesystem. This is much better then searching the entire modules directory. It could be improved even further by making a single well-known class in each namespace that declared and returned the list of classes with commands. I wish it had been implemented this way (maybe for Robo 2.x?). I similarly regret the command discovery class of the annotated command library, and will probably replace that with something better in the future.
I am not fond of #157. Searching for composer.json files and aggregating information from them reminds me of the old Composer Manager. The right way to get info from the `extra` section is via the Composer APIs, but those are only available to plugins, and it wouldn't make sense for Drupal core commands to be Composer plugins.
I like the direction that #167 is going. I have a couple minor comments, but perhaps the items below are only the way they are because this is a PoC.
-
+++ b/core/lib/Drupal/Core/DrupalKernel.php @@ -617,6 +618,10 @@ public function discoverServiceProviders() { + if ($this->environment === 'console') {It would be better if the ConsoleServiceProvider were somehow injected by the Console front controller rather than having a conditional here.
-
+++ b/core/lib/Drupal/DrupalConsole/CommandServiceDefinition.php @@ -0,0 +1,26 @@ + public function getDefinitions() {This isn't really a magic name; this is an explicit declaration of a command definition. I'm not sure how a module would extend this. I think it would be possible to replace this function with something that searched for well-known (magic) names though.
Drush only instantiates commands from enabled modules, for safety. I didn't try to instantiate commands without a bootstrap, but I agree with #155 that it would be safer to not do this. It might therefore be better to stick with established conventions and always bootstrap for module commands. Then, most commands won't need to worry about bootstrapping.
Yeah, okay, I could be +1 on using #152 with #147 -- forget about services and just pass the container into a static factory (create) method and let command classes pull whatever dependencies they need.
I don't think we need a marker interfaces for bootstrapping; I think it would be better to just have a bootstrapping trait (or even class) that commands could use to bootstrap if they need to. Previous patches on this issue had bootstrapping in a base class, but I think it would be better to keep the bootstrap utility out of the inheritance tree. Minor point, just code style.
Allowing module commands to be able to participate in dependency injection is an important feature. If we can somehow do that with vendor-discovered command classes, then I'm all in favor of that route.
Oh yeah I forgot to address #142. I don't think we really need to be able to run commands out of vendor at this point. It should be sufficient to provide a way to add commands to modules. If we did want Yet Another Way to define commands in Drupal, though, Robo provides an extension mechanism based on the psr4 autoloader and well-known namespaces that could be referenced for inspiration.
I think that #141 is the better option for Drupal core, though.
I like the discovery technique in #141. DrupalKernel::compileContainer() is already doing us the favor of collecting all of the module namespaces, plus the namespaces in Core and Components, so this seems like a natural facility for us to leverage. It is helpful that this mechanism works even without bootstrapping Drupal, so that commands that don't need a bootstrap can be executed quickly.
I think it is also important to allow commands to be constructed via the service container. Both Drush and Drupal Console currently do this; it is very useful for commands that already require a Drupal bootstrap to be able to utilize Dependency Injection for their commands.
It's also great that it is possible in #141 to make DrupalCommandInterface commands that do not require a bootstrap. It is possible to have the best of both mechanisms if the command dispatcher delays bootstrapping Drupal until necessary. Drush does this by extending the Symfony Console Application to bootstrap further in Application::find(), so that more commands can be found if needed. Of course, the downside is that "command not found" and "help" both need to bootstrap Drupal.
Some other comments about the issue summary:
This CLI integration should be viewed as a separate application which ships with Drupal. This might include going so far as to turn it into a subtree split similar to Drupal components.
Providing the CLI integration via a subtree split is not a good thing to do. This would create a separate global CLI application that would ostensibly be able to target multiple different Drupal sites. Both Drush and Drupal Console have shown that this is a Hard Problem. It is much better to consider the CLI integration to be a separate front controller on the SAME Drupal application / site. Bundling the CLI front controller with Drupal is the primary benefit to having the CLI in core; we don't really need Yet Another Global CLI to bolt onto the side of Drupal. We already have enough of those, and they work well.
Figure out the name. drupal/console project is already using the name 'drupal' as its bin file. Arm wrestle over it?
As pointed out in #131, core/scripts/drupal is the Drupal core CLI, and vendor/bin/drupal is Drupal Console (if it has been added to the site). There is no need for core/scripts/drupal to go in the user's global $PATH. Users might wish to put relative paths core/scripts and vendor/bin in their $PATH, in which case they could use drupal to run either Drupal Console or the core Drupal CLI per their preference. Most users will probably directly address the scripts (e.g. `core/scripts/drupal commandname`) or use a launcher. I therefore do not think this is a technical issue that needs to be solved in this issue.
I'd be happy to update the issue summary if there is general agreement on those points (or someone else can do it).
Yes, @moshe is correct; I mis-characterized item 5 in the summary in #107. It was not a goal of the BoF to decide what the script's name should be. There was a discussion about the problems that would arise if both core and Drupal Console installed a "drupal" script to /vendor/bin.
Note from the "CLI in Core BoF"
1. Commands in Core is a good idea. Some folks still hold reservations.
2. It is tbd how and when contrib modules should implement their commands as core extensions instead of as a Drush or Drupal Console extension. For the first release we should document this facility as "@internal" for at least one minor release.
3. Converting the existing core scripts would be a good place to start. It's a net benefit to be able to be able to get a list of all of the available cli commands available in core. Existing scripts could remain with their original name, but they would just call through to the new commands.
4. We need to be careful about how we promote it, so that folks realize that this does not replace (does not have as much functionality as) existing tools. Adding a third way to define module commands will be confusing for users in the short term.
5. The command name should not be called "drupal" because a lot of existing sites use Drupal console, and the Core "drupal" would conflict with the Drupal Console "drupal", both of which would install to vendor/bin.
6. It's unclear who would actually implement e.g. the port of existing Drupal Console / Drush commands, and whether this would gain traction.
EDITED only to fix numbering.
@alexpott I can't really mediate the use of the "drupal" namespace between core and console, but your idea is certainly technically possible. If you or any gatekeeper have any requirements to get this patch committed (e.g. a way for non-modules to add commands, maybe a way to do so and still participate in dependency injection, for example), then let me know, and I'll try to move this patch forward.
I might recommend, though, that the first version of a core cli released, at a minimum not use "drupal" as the command name. If this was done before Console had a chance to adjust, then everyone's CI scripts et. al. that depended on console would break. There was pretty strong consensus in the BoF that an abrupt b/c break of this sort would be undesirable.
@alexpott The issue here is that if Drupal core defined a command called "drupal", then any existing site that includes Drupal Console already (e.g. any site build with drupal-composer/drupal-project) will already have a file `vendor/bin/drupal`. When one of these sites use `composer update` to pull in the new version of Drupal core that also contains a `vendor/bin/drupal`, you will not be able to predict which `drupal` Composer will put in `vendor/bin`.