I did a search in core (granted, it was 10.2), for '#type' => 'textfield',, and stopped counting at 10 when I found files that rely on the default value.
Also, it seems like relying on the default value is kind of OK, which is why it's there.
Config offers a schema constraint of Length.max, so it seems like a config form would use that value if it's present, only using the form element default if there was no such constraint. Ideally this would happen in some automatic or semi-automatic way to make it easy to build config forms. Maybe as a method on ConfigFormBase that does magic for you.
It seems like there would be some other issue where this is happening, but I don't see it, other than in initiatives to make sure all config has constraints: 🌱 [meta] Add constraints to all config entity types Active It also seems like it would slot right into this CR's purpose: https://www.drupal.org/node/3373502 →
Searching through Drupal 11, I see that: A) LoggerChannelTrait is unused within core, and B) therefore no services follow this pattern. That's a step forward, I'd say... I assume the trait is still in use in contrib, which is why it's not deprecated at this point though maybe it should be.
Also, core now defines logger channels as services, rather than the factory, so you see this pattern in core.services.yml:
logger.channel_base:
abstract: true
class: Drupal\Core\Logger\LoggerChannel
factory: ['@logger.factory', 'get']
logger.channel.default:
parent: logger.channel_base
arguments: ['system']
logger.channel.php:
parent: logger.channel_base
arguments: ['php']
...
views.entity_schema_subscriber:
class: Drupal\views\EventSubscriber\ViewsEntitySchemaSubscriber
arguments: ['@entity_type.manager', '@logger.channel.default']
So yay, even better.
Destruct doesn’t happen in order during script shutdown so even if you had a reference to something it might have been destroyed already and become NULL.
Like this:
abstract class Connection {
public function __construct(object $connection, array $connection_options) {
// etc. etc.....
$this->transactionManager = $this->driverTransactionManager();
}
}
Now you always have a reference to the transaction manager for the life of the object, and the transaction manager will not be garbage-collected before the Connection is destroyed.
If PHP has decided that a referenced object can be destroyed through GC before there are 0 references to it, then we have a problem we can not solve here.
I wonder how many contrib tests will break because there aren't deprecated sub-traits remaining... Also DTT might have to do a re-organize.
Arguably it could go in Drupal\Tests\Traits but it's not clear when to use that and when not to.
The Traits namespace is there so we can limit the number of autoloaded test files per framework. Since some frameworks share traits, we put them in a separate directory so we can autoload them without autoloading everything else. This only applies to modules, AFAICR. https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/bootstr... See also TestSuiteBase and its subclasses. https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/TestSui...
It might help us to do the same thing for non-module core, but I doubt it.
From my MR comment:
It looks like the Connection object can have a reference to the transaction manager, which means it's not being destroyed, right? So if ($this->$transactionManager) { ... then use it, otherwise call connection->commit(). We'd look at the property rather than calling the service method ($this->transactionManager()) because otherwise we run into to the problem mentioned... The service method could end up generating a new transaction manager. Ideal solution: Actually inject the service instead of lazily generating one, so we always have a reference to it and it's not being destroyed before the Connection object.
I'm not sure about the internals of the DB layer, so maybe it's naive to say. :-) But just keeping a reference to things you expect to exist in your class seems like a non-breaking change that could be applied to past versions.
Setting to NW because even though the link templates docs are good (thanks!), they stop just short of telling me the spec for *.link_relation_types.yml files, or giving the context for how to figure out where to look. The two linked ones have URIs that are 404s, and then a description, so I suppose I can get away with just about anything, as long as the top-level key is defined, but it would be nice to know.
The other thing is this one: #2848945: EntityType link templates: using link relation type names as keys is a problem → which hints that maybe we should be careful how we define these, in line with an IANA standard, and it seems like referencing that standard in a relevant way on the docs page would be a good clue.
I'd update it, but I have yet to learn what to say. :-)
1) Derp... the ignore file is core/.deprecation-ignore.txt
2) Re: #4: Yes, I think this signals a coverage issue, because there's no ignore regex related to dynamic properties in any of the branches 10.0.x to 11.x.
I'm seeing this in Drupal 10.1.8 and PHP 8.2.13, running tests on my project, thanks to symfony/phpunit-bridge.
5x: Creation of dynamic property Drupal\Core\TypedData\TypedData@anonymous::$value is deprecated
5x in OURTEST::test from OUR\CLASS
(The @anonymous is because our use-case is weird... :-))
I can't find any reference to 'dynamic property' in core/phpstan-baseline.neon. Maybe I don't understand how it works or I have the wrong file.
Apparently this issue will make things break in PHP 9.0. I'd add a PHP 9.0 tag but it doesn't seem to exist yet...
smustgrave → credited Mile23 → .
Doing some triage.
drupal/core-dev requires composer/composer because we have Composer plugins in the repo that need to be tested.
See 📌 Add composer/composer as a dev dependency of core so that we can test Composer plugins Fixed
We also need to summarize decisions here in the IS, so a maintainer can review it easily.
+1 on #120. We should not be instructing people to make new packagist entries for their custom modules, and we should not ignore the custom modules directory.
So there's some discrepancy in the summary and title about which package we're talking about.
But:
If you say composer create-project drupal/recommended-project you'll get a project composer.json that doesn't use the path repositories. The same for drupal/legacy-project.
drupal/drupal does look like the issue summary, however: https://git.drupalcode.org/project/drupal/-/blame/11.x/composer.json?ref...
I believe this is because the drupal/drupal package is specifically for doing local development of core, and no other purpose. For core dev you're supposed to clone the repo and say composer install. Since the entire file system is laid out already in the repo, there's no need to use the scaffold plugin, so it is not included in the require section, and therefore also not included in the repositories section.
I'm not sure where to document this. I don't see a change record that says 'drupal/drupal is for dev only,' so maybe that's a good candidate, or maybe it already exists and I couldn't find it.
Asked just now on Slack, and @drumm says the GitHub split will happen automagically.
It seems like we need:
- Maintainer to say it's still in the cards.
- D.O ops person to reaffirm that a GitHub split will be created (automatically or otherwise) for the
drupal/core-attributespackage. - Add your own thing here... :-)
Then after that is sorted, we'd need to perform a re-roll, and can do the code review and changes.
With that in mind, adding 'needs subsystem maintainer review' since there's not really an 'ask ops' tag, and adding NRQI tag and asking in slack so maybe some maintainer can please affirm that it's worth the time to do.
#3272110: Drupal 9 and 10's Drupal\Component composer.json files are totally out of date → is in, so here’s this.
This patch changes the drupal/drupal root composer.json to require drupal/core 11.x-dev, but it’s the only way I could get it to work. This might imply that we need to specify the major/minor dev branch for each branch, which could be a drag.
Even though it's fair to say these should be two separate issues, these are both bugs, because hyphen is legal in table names and should not be a special case.
In fact, the reason I'm commenting here is because our project (DKAN) can create and delete a table with a hyphen in the name, but it can't be used in a DBAL query. See here: https://github.com/GetDKAN/dkan/blob/2.x/modules/common/src/Storage/Abst...
We see errors like this, against a table name with a hyphen:
drush dkan:harvest:run hospital-summary
In ExceptionHandler.php line 46:
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'db.harvest_hospi
talsummary_runs' doesn't exist: SELECT "t"."id" AS "id"
FROM
"harvest_hospitalsummary_runs" "t"; Array
(
)
In StatementWrapperIterator.php line 110:
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'db.harvest_hospi
talsummary_runs' doesn't exist drush dkan:harvest:run hospital-summary
In ExceptionHandler.php line 46:
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'db.harvest_hospi
talsummary_runs' doesn't exist: SELECT "t"."id" AS "id"
FROM
"harvest_hospitalsummary_runs" "t"; Array
(
)
But querying the DB we see this:
mysql> show tables like 'harvest%';
+---------------------------------+
|Tables_in_db (harvest%) |
+---------------------------------+
|harvest_hospital-summary_hashes|
|harvest_hospital-summary_runs |
|harvest_plans |
+---------------------------------+
3 rows in set (0.01 sec)
This means that Drupal's DBAL created the table, but the table name is somehow modified during the query.
This is using Drupal 10.0.11.
❤️ ddev!
Note that it would be relatively easy to create a ddev-addon for Drupal core development, to perform tasks like running the CS checker, run-tests.sh, and setting up an environment for stuff like javascript functional tests.
So the project here might be to spec out an add-on.
Another issue is that we don't want to 'pollute' the core composer.json, so we'd need the path repo or similar approach to including core in the dev environment. I believe https://github.com/joachim-n/drupal-core-development-project does this. I think there might be other projects as well, which do this.
volkswagenchick → credited Mile23 → .
run-tests.sh is notoriously difficult to modify, update and, frankly, use
See this meta: #2626986: [meta] Improvements to run-tests.sh →
I generally support @catch's idea of culling out stuff from run-tests.sh that isn't in use by CI processes. But I also think it's not a huge lift to make it more maintainable, minimized or not, as per #2624926: Refactor run-tests.sh for Console component. → (which won't be getting a re-roll by me until there's some consensus). It should be even easier after the work for #3057420: [meta] How to deprecate Simpletest with minimal disruption →
The question here is not that there are tradeoffs between paratest and run-tests.sh, and which will we choose? The problem is that no one's really maintaining run-tests.sh, for fearful reasons rather than engineering ones.
Put it into a component, and for realsies, run-tests could be the coolest thing we export to the PHP world at large, but instead it languishes in a fake shell script.
Setting this issue to be a child of the meta.
Patch no longer applies towards 6.0.1.
+1 on baby steps to annotate these tests, and then follow through with converting other tests to use API calls in other issues.
My one very minor gripe is that @covers has a specific meaning for PHPUnit, such that all the coverage in a coverage report for that test will be filtered so it only contains that class or function. So this is a little bit of an abuse of the annotation, but is also completely reasonable for this process.
Some of these tests could likely be converted to KTB, but that's another meta for another day.
Very curious how the determination was made per test... Is there some magical regex or other methodology to generate a list like this? We could apply it to other settings forms, in order to find places to convert form-fill actions to API calls.
Related but super-low priority: #2624926: Refactor run-tests.sh for Console component. →
👍
Maybe make issues about the db and prefix stuff if it's worrisome, and soft-postpone this on that...?
@jastraat mentioned that she couldn't reproduce the fail, so I ran it again and it passed.
It would be *very* much better if a maintainer could turn on automatic CI for this project.... :-)
Verifying that the patch in #11 still applies and doesn't seem to break anything. :-)
Using a path repo setup in a Drupal core dev environment, I was able to verify that I can install and enable the patched module with core 9.5, 10.0, and 10.1. Also was able to edit text and add shortcodes.
It would be super-nice if the dev branch were passing tests, and we could see changes to tests here.... I don't see another issue to fix those so let's do it here, unless someone wants to file it.
The test changes in the patch do (mostly) fix the tests so they'll run again.
I had passing tests for Drupal 9.5x and 10.0.x.
Using Drupal 10.1.x I only saw one fail:
$ ./vendor/bin/phpunit --config ./core/phpunit.xml --group shortcode
PHPUnit 9.6.8 by Sebastian Bergmann and contributors.
Testing
........F 9 / 9 (100%)
Time: 02:31.670, Memory: 158.00 MB
There was 1 failure:
1) Drupal\Tests\shortcode\Functional\ShortcodeTest::testRandomShortcode
Random shortcode self-closed, output length is correct.
Failed asserting that 6 matches expected 8.
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/shortcode/tests/src/Functional/ShortcodeTest.php:400
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
Reroll for 11.x looks good, my concerns from #63 about @see are addressed.
Setting RTBC.
Hopefully we'll be able to backport this to 10.0 and/or 9.5.
I want to give a special tangential shout-out to ✨ Refactor cron queue processing into its own class Needs work because I just spent a bunch of time trying to figure out a bespoke queue implementation that should have been an easy subclass of a generic queue handler. Tangent over.
I suggest this should be more formally part of the Queue API (rather than cron), under Drupal\Core\Queue namespace and the service named queue.processor.
Also +1 on #11.4.
This looks like a duplicate of ✨ Split \Drupal\Core\Cron::processQueues into its own class Closed: duplicate which has work.
I think the issue here is that we need some specialist information, such as whether the infrastructure is set up to add another packagist listing and whether the subtree split will work.
Back in the day, @Mixologic had that set up to happen smoothly, but since he's not at the DA any more, it unclear who to ask. Or at least I don't know who. :-)
Gábor Hojtsy → credited Mile23 → .
Drupal core currently uses stylelint to lint CSS files.
You can see these files excluded in .stylelintrc.json here: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/.stylelintrc...
They are used for testing, as seen by the fact that they live in core/tests/.
So it's highly likely that they are the way they are for a reason, and should remain that way.
Setting to Postponed. If this issue does not move forward in a few months, close it.
It seems that there's already a commit for this issue at https://git.drupalcode.org/project/select_or_other/-/commit/ae0585ff8cc5...
This issue improves PHP 8.1 compatibility for tests and is worthwhile.
It'd be great to have a roadmap to a release that supports Drupal 10, but for now 4.x-dev works, and this patch enhances it.
Verified that the patch applies, and that it can be installed using Drupal 9.5.x and 10.0.x, using both PHP 7.4 and 8.1.
Recent fail seemingly unrelated:
Testing Drupal\Tests\text\Functional\TextFieldTest
....F 5 / 5 (100%)
Time: 02:12.704, Memory: 4.00 MB
There was 1 failure:
1) Drupal\Tests\text\Functional\TextFieldTest::testTextfieldWidgetsAllowedFormats
Failed asserting that '\n
uzTtfezy\n
\n
\n
oKFzcW4S_label\n
Hello World\n
\n
' contains "".
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/text/tests/src/Functional/TextFieldTest.php:261
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
Rebased and pushed again for another test.
@catch: The downside of leaving this bug in core is that a developer might composer install without dev for some reason and make a patch that breaks the metapackages. Then a maintainer might end up not noticing that. It seems vaguely unlikely, but it also seems like it would be a real pain to figure out if it broke something else.
Kristen Pol → credited Mile23 → .
Updating IS to reflect the blocker.
Based on #20 and #21.
Drupal\Tests\system\Functional\Datetime\DrupalDateTimeTest currently says this in setup:
/**
* Test setup.
*/
protected function setUp(): void {
parent::setUp();
}
So that's a bug in that it's an unnecessary function, which is different from the scope here. But it certainly solves this issue. :-)
There's also a unit test for Drupal\Core\Datetime\DrupalDateTime which fills me with a warm glow.
This is a duplicate of 💬 2.0.0 release w/ Drupal 10 support Fixed
I think this is premature, because we need to change simpletest_script_run_one_test() per #5.
"The only downside with this approach is that you need to commit changes in the checked-out contrib module and then run composer update YOUR_MODULE_NAME to have them reflected."
Not sure what this means. Using a path repository, Composer will make a symlink from your local git repo to wherever the module belongs. Since that's the case you don't need to update anything to have it reflected in the Drupal codebase; it's already there.
Patch in #39 no longer applies. Using git apply --rej shows us that all changes apply except the one to core/phpunit.xml.dist, and then passes composer phpcs. So it should be easy to reroll.
MR in #35 seems broken.
In an ideal world, the MR would reflect the current changes, but it might be less work just to stick with patches. Dunno.
Re: #76
We use run-tests.sh in the CI right now, and it would be great if it were more maintainable.
IIRC the reason it's got the .sh extension is that PHP won't discover the file, and so that htaccess type configuration can exclude it more easily. So it's a security through obscurity type scenario.
Changing the file name also means changing the CI system, and while it doesn't seem like we'd need to alter too much, we'd end up with different file names for different versions of core. A CI system maintainer could speak to whether that effort would be worth it, and what the future plans are for switching to a different test runner potentially.
In my ideal world this script would have been refactored into a symphony console based app, and thus much more maintainable and less ambiguous when it comes to file names. #2624926: Refactor run-tests.sh for Console component. → But I fear that ship has sailed as well, because we have always been 'just about to' switch over to something else.
The current behavior is that for dev branches, the project templates include drupal/core-dev as a require-dev. For tagged releases, drupal/core-dev is excluded.
Release:
% composer create-project drupal/recommended-project:@stable
Creating a "drupal/recommended-project:@stable" project at "./recommended-project"
Installing drupal/recommended-project (10.0.2)
- Installing drupal/recommended-project (10.0.2): Extracting archive
[...]
% cd recommended-project
% composer show "drupal/core-*"
drupal/core-composer-scaffold 10.0.2 A flexible Composer project scaffold b...
drupal/core-project-message 10.0.2 Adds a message after Composer installa...
drupal/core-recommended 10.0.2 Core and its dependencies with known-c...
Dev:
% composer create-project drupal/recommended-project:@dev
Creating a "drupal/recommended-project:@dev" project at "./recommended-project"
Installing drupal/recommended-project (10.1.x-dev f2de0da24e4fdae1ae8b81d2f4f3fbd038809017)
- Downloading drupal/recommended-project (10.1.x-dev f2de0da)
- Installing drupal/recommended-project (10.1.x-dev f2de0da): Extracting archive
[...]
% cd recommended-project
% composer show "drupal/core-*"
drupal/core-composer-scaffold 10.1.x-dev 14db477 A flexible Composer projec...
drupal/core-dev 10.1.x-dev a6812ad require-dev dependencies f...
drupal/core-project-message 10.1.x-dev 59b4475 Adds a message after Compo...
drupal/core-recommended 10.1.x-dev 3981ea2 Core and its dependencies ...
So in the meantime we split the difference: Releases don't risk including dev requirements by default, while dev setups include the dev tools.
The question then becomes: Should we exclude the dev tools from the dev project, and it's either +1 for consistency, or -1 for not letting users decide to take fate into their own hands by using @dev without having to type composer require drupal/core-dev
I think I'm +1 for consistency, where we just never add drupal/core-dev to the project templates, and we tell users to add the package they need. Obviously real actual maintainers might have a different view. :-)
Since we have a CR, setting the issue to target Drupal core 10.1.x.
Re: @jonathan1055 #43.c:
This issue adds a feature that isn't on any maintainer's roadmap, as far as I can tell.
Once upon a time there was #2624926: Refactor run-tests.sh for Console component. → which would refactor run-tests.sh and make it easier to test the test runner, but that's fallen way behind.
Core maintainers have expressed an interest in having coverage reports, but clearly it's not a priority.
One can always run tests using the phpunit test runner and generate coverage for the parts of code you're interested in. For all of Drupal Core this would take a very long time, but it's doable. It's also doable per-module, for instance, for your contrib module or what-have-you.
Here's a re-roll-ish for 10.1.x. When I run it, phpcov runs out of memory for a very small set of tests.
If anyone wants to pick this up, they're welcome to it. :-) I don't have much time to dedicate to it these days.
Setting to needs review to get a test run, but really the next person to come along is welcome to set it back to needs work.
Given #23 let's call this issue postponed on ✨ Allow run-tests.sh to generate coverage reports Needs work . We currently don't do a coverage report for core for the project CI, AFAICT.
The patch in #11 no longer applies. The reason is that Drupal core now uses this line to do the directory discovery, after #3035312: Move file_scan_directory() to file_system service → :
foreach (\Drupal::service('file_system')->scanDirectory($directory, '/\.php$/', $ignore) as $file) {
That is, the system under test is discovering the tests.
That means that while there's probably test coverage for the file_system service, it also means that the test coverage for the file_system service is potentially discovered by the file_system service.
Therefore it's impossible to ascertain whether the goal of test coverage for run-test's --directory feature is met, though we could probably just go on assuming our quality is high without any data, which seems to be The Drupal Way.
It's also unclear whether anyone really wants to go about gathering the data which would help us understand the quality of the product, so I'm going to set this issue as Active. If anyone wants to champion this cause they can pick it up.
The future we want: 🌱 [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … Active :-)
OK, to recap:
1) There's Drush. :-)
2) There's Drupal Console, which is a third-party console app which is still minimally supported.
3) There is this issue, which really is very, very old. It was prompted by having Drush not work with a specific Drupal vs. PHP version (IIRC), so the idea was to have some of that stuff live in core, where it could be maintained alongside core itself, instead of as a third party. This issue is also from a time before the Ideas project, and should probably be marked as duplicate of....
4) #3089277: Provide core CLI commands for the most common features of drush → is an Ideas issue which wants to minimally replace some Drush behaviors with native Drupal console-based commands, without getting bogged down in implementation details as this issue does. However, it did get bogged down in the same kind of inertia that this issue did.
And now we apparently also have:
5) ✨ Add drush/drush to Drupal Composer project templates Active which isn't such a bad idea, but again suffers from a potential lag time where Drush might not be compatible with core or its dependencies for some reason. Our community's release schedule and process has improved drastically since 2014, so I doubt it's nearly as much a concern today.
Marking this issue as postponed, because if someone reeeeeeeaaaaallly needs me to re-roll #193 I will. However, effort and conversation should happen on #3089277: Provide core CLI commands for the most common features of drush → and/or it's children, which are the spiritual successors of this issue.
There's some confusion because there is a project called drupal/console, which is not the 'official' Drupal console.
There *is* a console command that ships with core, at /core/scripts/drupal, which is very likely where any work in this issue will end up.
Reroll for 9.1.x.
#3089277: Provide core CLI commands for the most common features of drush → suggests this issue should become a meta.
I'd suggest that a meta be a brand new thing where we can track the actual plan. This issue is more of a proof-of-concept.
Will set back to 'postponed' after the tests finish.
That's the old patch.
Here is the new one.
This is a re-roll of #167 against Drupal 9.0.x.
This is presented so it's easier to evaluate what's going on here. Real work is happening in #3089277: Provide core CLI commands for the most common features of drush → and not here. Marking this issue postponed.
This does not address any of the comments since #167 (and maybe not even some before that).
Targeting 9.0.x because it's super duper highly very improbably unlikely that any of this will end up in core before then.
Woops. Wrong link.
#3089277: Provide core CLI commands for the most common features of drush →
We now have an idea process starting up for CLI in core type stuff: #2242947: Integrate Symfony Console component to natively support command line operations →
Class loader collects prefixes not namespaces.
It collects whole class names if they're part of a classmap. Otherwise, yes, it's prefixes. But our naming convention would use the prefix to discover the class. That's what #167 is doing.
So a prefix like Mile23\Something\ gets reduced to Mile23\ in order to figure out if Mile23\DrupalConsole\ConsoleServiceProvider exists. If the service provider doesn't have an appropriate prefix in PSR-0/4 or classmap, then it can't be autoloaded anyway.
Can we add a setter to the Kernel?
We want to perform discovery and container manipulation during the container compile time. DrupalKernel has that roped off, and it'd be pretty complex to change that. Also, as you say: We're core, so we can add a conditional. :-)
How would a module add a command with this mechanism?
I guess it might not be obvious from the patch.
In #167, if you want to add a command to your module, you add it as a service to your modulename.services.yml file and tag it with console.command. If the module is enabled, the command is discovered. This is just like any other service you'd get from a module.
If you want to make a composer-based package instead, you use a class named YourNamespace\DrupalConsole\CommandServiceDefinition that implements DrupalConsoleServiceDefinitionProvider. That ends up looking like this:
public function getDefinitions() {
return [
// Service name.
'console.run_cron.command' =>
// Service definition based on class name.
new Definition(RunCron::class, [
// References to other services, equivalent to arguments: in YAML.
new Reference('console.bootstrapper'),
]),
];
}
As I mentioned in #167, we could provide some boilerplate that parses a YAML file and then satisfies DrupalConsoleServiceDefinitionProvider, and then people could just use YAML.
Re: #169:
1: We can't inject the service provider because the kernel doesn't give us any opportunity to.
2: The function isn't a magic name. The magic name is the name of the class we discover. I guess I should call it a naming convention.
This demonstrates locating services by using the classloader to find service providers with magic names.
If you apply this patch locally, be sure and say composer dumpautoload before you do php ./core/scripts/console list. You'll see drupal:cron in the list, which is discovered through a magic-named service provider factory type thing. :-)
The tests will still fail because I didn't fix them here.
There are plenty of coding standards errors, but this is PoC.
The downside here is that in order to provide a command from a dependency that isn't a module, you have to know how to make a Symfony DIC service definition in code. It's not terribly hard, but it's less convenient than YAML. We could provide a boilerplate service provider that just reads in an arbitrary service definition YAML file from within the package in vendor. That would make it easier for DX.
I could have sworn I changed that file name....
In #162 we're not using console.services.yml. It's the kernel's container.
Here's a patch that uses DrupalKernel to initialize the container enough to discover services, and then assemble the console app with the tagged commands. Moving to using translation will be enabled by getting @string_translation through the container.
It can't discover services for modules which aren't enabled, because why would it? :-) There's a POC of this for the clear cache command, which I put in the system module. If you perform an installation, the system module will be available, and its command is discovered.
It does not discover any commands in vendor, because there are only so many hours in a day. The plan might well be to add another bit to the ConsoleServiceProvider which does whatever scan is necessary.
I spent a little time poking through drupal/console looking at their translation system. It's not using Drupal core, but it's good for them. I'm still trying to figure out where the actual string swaps occur, because that's where ours should, as well.
Interdiff from #136.
Edit: Woops. Left over service definitions from the old patch. Will need to change that.
OK, so how do we implement multilingual for a site that isn't installed, so that you can install it from the command line?
Maybe we need to go back to the installer kernel idea for command discovery. This gives us services and allows a multilingual install process without an existing site.
Then commands that need an installed Drupal to work can instantiate their own DrupalKernel (via a base class which we provide).
-1 on adding to composer.json extra sections. That's for Composer scripts, not our use case. It would also require too much setup for a given command to work out of the box.
I would like to have command discovery in vendor though it won't be a trivial task.
We don't need to do file discovery. If we have the class loader, we can ask it for all the namespaces and then figure out if any of them contain a [YourNamespace]\Drupal\Console\CommandFactory class that implements \Drupal\Core\Console\CommandFactoryInterface.
Then we instantiate the factory and it gives us all the commands.
And since we're using a booted DrupalKernel and using its container, we can then move on to just tagging services as console_command or whatever, and discovering them that way, and that's how we discover commands in modules.
So that's two cases: vendor we look for classes with magic names, and modules and core define a service with a special tag. That's not too complicated, is it?
+1 on #143, #144, and the good work up to and including #141. I had concerns about a bootstrapped DrupalKernel as in #141. We might even switch to use the installer kernel just to do autoloading, but it might not matter.
We might even somehow factor the extension namespace loader functionality out into its own domain, so we can just do that without a kernel. That might be too radical a change, though.
I like the idea of discovery in vendor better than I like the idea of discovery in modules. :-) But I'm mostly ambivalent about it. The main goal is to have functionality in core, that tests along with it, that doesn't fall out of sync with dependencies and always works.
I was looking at Robo to find out how you guys did the extension system. I think the Robo model for that might be the best for having commands in vendor, so we can use a \MyNamespace\Drupal\Console\Command\ThisIsTheCommandClass type naming convention. But again, somewhat ambivalent as long as we can come up with something that core maintainers might like and/or enjoy.
Refactored the cache clear command a little so it's easier to unit test. These commands are mostly proof-of-concept, so we don't need a lot of coverage at this point.
Fixed deprecated PHPUnit mocking.
Fixed CS.
Making the tests pass.
It's a separate container. It should not use any core services.
But, IMO, we've solved the issue title "Integrate Symfony Console component to natively support command line operations" -> that is done.
It all depends on what we mean by 'integrate.' The patch gives us a few different behaviors that we currently don't have:
- Booted Drupal, as you mention.
- Discovery of commands as services, even in extensions. Meaning to add stuff you just make a command class and put it in the right place.
So if those are valuable parts of integration, then we can rescope here to add those. Followups might be needed to convert existing commands to this API.
It might also be true that if we want to move forward on this, we'd need to go through the core ideas process → , which didn't exist when this issue was first filed.
This is not so good a solution because it yields a required marker on all the elements the user could choose from. That could be confusing.
Edit: On second thought, this might be our CSS.
Reroll #22 for 8.8.x.
This worked for me just now:
$ git apply 2242247_121.patch
$ git checkout -- composer.lock
$ composer update symfony/finder
$ ./core/scripts/console drupal:cache-clear
This issue is really old and doesn't have a lot of consensus, so it's probably not worth working on it very hard.
Thanks for the update, @greg.1.anderson. :-)
Work on #2911319: Provide a single command to install & run Drupal → shows that we're comfortable with one-off scripts using console to provide UX. There are other console-based scripts in core which aren't really user-facing so much, like #2926633: Provide a script to install a Drupal testsite → which exists for tests.
That leaves us with a few decisions:
- Should extensions be able to define console commands? My answer: Yes.
- Should we, for instance, make the console a dev requirement that can be uninstalled in production? Maybe have a dev 'component' and a non-dev 'component', so that for instance the new demo installer script can always be present even if maybe-harmful commands are not.
- What testing standards should we implement for console scripts, whether they are part of an integration or stand-alone? We should have standardized ways of testing commands, so that we can use them uniformly in all scripts.
Definitely needs to use SymfonyStyle.
Better structure for the issue summary.
Soft-postpone on #2911319: Provide a single command to install & run Drupal →
Updating IS with some implementation details.
...Except that the reason we get to that code path is that there's no kernel. And if the kernel can't be generated, we already do this:
+++ b/core/lib/Drupal/Core/Console/Bootstrap.php
@@ -0,0 +1,70 @@
+ catch (\Exception $e) {
+ /** @var \Symfony\Component\Console\Helper\FormatterHelper $formatter */
+ $formatter = $command->getHelperSet()->get('formatter');
+ $error_messages = array(
+ 'Insufficient Drupal to proceed.',
+ 'This command must be run within an installed Drupal.',
+ $e->getMessage(),
+ );
+ $formatted_block = $formatter->formatBlock($error_messages, 'error', TRUE);
+ $output->writeln($formatted_block);
+ }
+ return FALSE;
We should implement a style object in order to pipe the message to stderr.
Making ClearCacheTest pass after this change involves converting it to a KernelTestBase test, (because drupal_rebuild() is not actually mockable (meaning not testable)), which also involves creating a KTB test base class for the console app.
This is all work I'll gladly do if this moves forward, but the original issue was filed in 2014 and there's a year-and-a-half gap before #88. There's enough here to evaluate whether it's a thing anyone else wants.
Hah. I made the changes for #91 but never uploaded the patch.
Interdiff here is to #88.
This testbot build might break because composer.lock isn't up to date. That's because of strange vagarities of the computer I'm using necessitates modifying a bunch of stuff that means a wrong content hash.
I wonder if it is a good idea to create ./bin or ./core/bin directory for command line scripts?
We'd want to place console into a reasonable place, and then specify its bin status in core/composer.json. Then Composer will alias it somewhere when we use it. (That's how, for instance, phpunit ends up in vendor/bin/phpunit.)
Moved console script to core/scripts.
Eventually, we'll have to figure out how to specify bin status for core/scripts/console. We can't do that now because of the way we merge core/composer.json with the wikimedia merge plugin. The alternative is to do it in the root composer.json file, but that's not really where it should be.
Updated for 8.5.x and with some coding standards stuff. Probably more is needed.
Re-rolled because of #2906637-30: [META] Drush and core compatibility is fragile →
:-)
Based on that issue, adding a command-line updater would be warranted.
There also might be some hope for composer-averse users as well, but, for instance, we should not ever add composer/composer to our list of production requirements (dev maybe).
This patch contains a cache clear command, and a run cron command, so reviewers can get a sense of how it works. Here's what it looks like when you run those commands:
$ php ./core/console drupal:cron
Running cron...
Done.
$ php ./core/console drupal:clear-cache
[Symfony\Component\Console\Exception\CommandNotFoundException]
Command "drupal:clear-cache" is not defined.
Did you mean one of these?
drupal:cache-clear
drupal:cron
Grant:drupal paul$ php ./core/console drupal:cache-clear
Caches cleared.
$
'Need' is a strong word. :-)
We already require symfony/console for core, so that we can make commands like that using the console framework. The main problem is that you then have a lot of repeat effort for things like output styling, for instance.
I already refactored run-tests #2624926: Refactor run-tests.sh for Console component. → but no one seems to care. :-)
If this were in, would it allow us to start refactoring the various scripts in code/scripts to use console?
Convert existing core/scripts/ to Console you say? :-)
#2624926: Refactor run-tests.sh for Console component. →
Basically what this integration gives us is a discovery mechanism and a unified UX at the command line. It'd be nice to have a unified single console app with lots of commands, but existing scripts could just be converted as desired.
Also, we could just say composer require drupal/console because a lot of the problems have been solved there. :-)
As for symfony/finder, it's very convenient but yah, no real attachment to it. The problem here is how to do discovery before booting the kernel (since some commands won't need the kernel). We also want to search in vendor/, or come up with some elegant discovery mechanism that includes vendor/, so that stand-alone commands can just be required using composer.