Problem/Motivation

Over in #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible, the direction proposed requires that the Symfony Console component be used for the database generation script. This will make the script testable and more concise.

From amateescu:

Discussed a bit in IRC with @alexpott and he pointed me to some previous discussion about it: #2242947: Integrate Symfony Console component to natively support command line operations

So just to be perfectly clear, I'm not proposing anything like that since it's definitely 8.1 material. I just want to write the code for the dump script in a Command so we can take advantage of the clean API for input options and arguments, nicely formatted output, and, why not, testability of that command. Then have core/scripts/database-dump.sh as three lines of code that executes the command, that's it :)

Proposed resolution

- Add symfony/console component to Drupal 8 core

Note:

The same as with BrowserTestBase, conversions of existing code are "prohibited" and new code using that component is judged on a case by case basis by the core committers.

For 8.1.x conversions are fair game then.

Note2:

This does NOT add a console to Drupal, but it rather allows scripts to not have to reinvent the wheel.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because the policy is to have Dries' OK when adding external dependencies.
Issue priority Major because this issue is a blocker for a Critical.
Unfrozen changes Unfrozen because this issue supports creating automated tests of the update system.
Disruption Not disruptive.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
3.11 KB
741.51 KB

Status: Needs review » Needs work

The last submitted patch, 1: console-compnent-2493807-01.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
746.82 KB

Patch required a --binary option to diff.

Mile23’s picture

+1 as an understatement.

The use-case in that quote from @amateescu is a perfect example of the reason I started on #2242947: Integrate Symfony Console component to natively support command line operations. The problem with that issue is that it lacks specificity. Please do it here or there or wherever, so we can make structured, unit-testable scripts to do things. Much preferable to php scripts with .sh file extensions with lots of implicit dependencies. :-)

Note that Console has its own miniature testing framework, so let's not reinvent the wheel there, either: http://symfony.com/doc/current/components/console/introduction.html#test...

larowlan’s picture

larowlan’s picture

benjy’s picture

This isn't a duplicate because 2242947 is about having commands in core for things like clearing the cache. This issue is simply to bring in the symfony/console component which will be used to replace a shell script in #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible which would normally run outside of Drupal.

Mile23’s picture

Issue summary: View changes

Adding a beta evaluation.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community
diff --git a/core/composer.json b/core/composer.json
index 5f64514..3f33143 100644
--- a/core/composer.json
+++ b/core/composer.json
@@ -31,7 +31,8 @@
     "behat/mink": "~1.6",
     "behat/mink-goutte-driver": "~1.1",
     "fabpot/goutte": "^2.0.3",
-    "masterminds/html5": "~2.1"
+    "masterminds/html5": "~2.1",
+    "symfony/console": "2.6.*"
   },
   "autoload": {
     "psr-4": {

This is the point of leverage for the patch. It doesn't add any native code, only the console component.

I'd argue that we should move the entry for symfony/console up near the other symfony components in the file, but that's only a cosmetic change.

The differences between this issue and #2242947: Integrate Symfony Console component to natively support command line operations have to do with the other issue doing more to integrate the console with the kernel and container. In that one, the console is a service and has its own discovery methods and so forth.

In this issue we are simply adding console as a dependency, so scripts can use its abstractions to everyone's benefit without integration into the kernel and the container. This improves maintainability and testability and general DX for all scripts moving forward, and still leaves us free to approve or deny the integration at a later date.

#2242947: Integrate Symfony Console component to natively support command line operations is marked as postponed until 8.1.x. This is reasonable, because it adds features.

This issue should not be postponed, because writing better support scripts should start now.

Since the patch still applies and I think we should get maintainer eyes on it, I'm marking RTBC.

larowlan’s picture

On my phone, so difficult to tell for sure - did this go in as require-dev? I'd argue it should

Xano’s picture

On my phone, so difficult to tell for sure - did this go in as require-dev? I'd argue it should

It went in as require. ./core/composer.json does not have a require-dev section and as such specifies dependencies like Mink and PHPUnit in require as well. The reason for this is that Drupal is still (mainly) installed by downloading the tarball and the current majority position is that we cannot force site builders to use Composer–in this case to install require-dev dependencies.

Crell’s picture

Even if core uses it only for dev commands, I can easily see contrib leveraging it for non-dev commands. Let's leave it as a require and let Contrib do what it does best: Come up with things we never thought of. :-)

benjy’s picture

The reason for this is that Drupal is still (mainly) installed by downloading the tarball and the current majority position is that we cannot force site builders to use Composer–in this case to install require-dev dependencies.

Mink and PHPUnit seem like dev dependencies to me, they're not needed for everyday usage of the site so why would they even need to be in the tarball?

Even if core uses it only for dev commands, I can easily see contrib leveraging it for non-dev commands

Sure, but if that's the case, the contrib module should be the one specifying the dependency on the symfony/console component anyway.

I agree it's probably not for this issue to sort out since PHPUnit and Mink are already under require but we should definitely have that discussion in a follow-up.

larowlan’s picture

webchick’s picture

Pinged the Drush issue about rolling in Console support https://github.com/drush-ops/drush/pull/1337 to see if they have any thoughts.

I can definitely see the rationale for this, as long as we're careful about not going off on a binge trying to re-write all of our .sh files before 8.0.0 (8.1.x+ is fine for that, though). Thanks for making the distinction between this issue and the other more clear.

Fabianx’s picture

Assigned: Unassigned » Dries
Issue summary: View changes
Issue tags: +Project governance

Adding "project governance" tag per our policies and assigning to Dries.

The same as with BrowserTestBase, conversions of existing code are "prohibited" and new code using that component is judged on a case by case basis by the core committers.

webchick’s picture

Assigned: Dries » Unassigned
Issue tags: -Project governance

Actually, external libraries don't need Dries's sign-off anymore, as of our newfangeldy core governance structure. It's a framework manager-level decision. Alex Pott seemed to be on board, and there were no other concerns with the other core committers when this was raised with them the other day on a triage call, so I think we're good there.

I would just commit it myself, but figured it best to make sure this stays at RTBC a bit longer than 1 day over a long weekend. :)

dawehner’s picture

Issue summary: View changes

Actually, external libraries don't need Dries's sign-off anymore,

This is the best thing ever!

Added a short clarification to the issue summary, what this is about.

jmolivas’s picture

This make total sense

Since we have been working on this contributed project http://drupalconsole.com/ there is already code that could be reused on core or contributed space to facilitate the use of Console component in order to enable Drupal users to create commands in a easy way.

@jhedstrom me and the rest of the console maintainers @enzolutions, @omers and @dmouse are interested in helping you with creating console commands, maybe we can help you to contribute new commands to Drupal Console or help you creating commands in a custom/contributed module within your project, don’t hesitate to contact us.

Xano’s picture

Sure, but if that's the case, the contrib module should be the one specifying the dependency on the symfony/console component anyway.

That's still quite tricky. See #2477789: Use composer to build sites.

jmolivas’s picture

FileSize
1.94 MB

@Xano: This is not totally true:

Sure, but if that's the case, the contrib module should be the one specifying the dependency on the symfony/console component anyway.

Modules do not need console as dependency, you can avoid setting the dependency and coding the bootstrapping if using the console project to execute the generated commands within the contrib module.

As you can see in this example:
generate-command.gif

jmolivas’s picture

Mile23’s picture

+1 in spirit on that console project in core, but it's outside of scope here.

@amateescu needs a tool so he doesn't have to reinvent the wheel in order to make a script so that Drupal is more testable.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: console-component-2493807-03.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

+1 Mile23

@jmoilvas
I think everyone knows that you are working on the Drupal console, let's focus on getting the component in and solve the critical issue.

cilefen’s picture

Status: Needs work » Needs review
FileSize
738.84 KB
cilefen’s picture

Issue tags: -Needs reroll
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

greg.1.anderson’s picture

TLDNR: +1, but I think it would be helpful to review a shell script that has been migrated to use symfony/console in core as part of the decision process.

There are two considerations here:

1. Does this provide benefit (that is in scope for 8.0)?
2. Are there complications related to adding this component, and if so, is there a better way to do it?

From the above discussion, it seems that there is benefit to just adding symfony/console to D8 core vis-a-vis various existing shell scripts, some of which could be written as .php files that used the Symfony Console. It would be useful to see this in action (e.g. to compare what the script looks like compared to the same sort of functionality implemented in Drupal Console), but it looks like it could be valuable in contexts where we do not want to make Drupal Console or Drush a requirement.

Complications have been discussed (c.f. https://www.drupal.org/node/2477789#comment-9954773 and follow-on comments). The Symfony/Console take on this is, that with Symfony/Console in core, it makes it hard on the Drupal Console project, which has to have its own copy of Symfony/Console when it starts up, and it's going to get another copy of Symfony/Console when it bootstraps D8, and pulls in Drupal's autoload.php. However, Drupal Console actually already has this problem, because Symfony/Console requires symfony/event-dispatcher, which is also required by Drupal 8. The current solution employed by Drupal Console is to insure that their components are exactly in sync with Drupal core's requirements. This means that Drupal Console is revision-locked to a single version of Drupal; if you tried to use it with any other release, you could end up mixing classes from old and new versions of symfony/event-dispatcher, which is likely to cause critical failures.

Drush has the same sort of issue, only indirectly; although Drush itself is not using components also used in Drupal core (except symfony/yaml, which is likely to remain stable), Drush extensions might use any sort of library, including symfony/event-dispatcher, or anything else. The interim solution we have in mind for Drush is the site-local Drush issue (https://github.com/drush-ops/drush/pull/1343), which basically proposes that Drush users 'require' Drush in their Drupal 8 site's composer.json file (n.b. this is not a proposal to add Drush to core/composer.json), and then relies on the global Drush to provide the mechanism for finding and launching the correct local Drush.

All of this is still in development, but moving symfony/console into core looks like a step in the right direction.

jhedstrom’s picture

re: #29.1

1. Does this provide benefit (that is in scope for 8.0)?

The main benefit is that the script required to get one of the remaining critical issues blocking the beta-to-beta upgrade (#2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible) would be testable, and can leverage all the formatting, input options, and argument handling that the console component provides.

greg.1.anderson’s picture

Yes, but the script in that issue does not currently use any of those features. I thought it might be helpful to see the proposed changes in action. I'm fine with moving forward and committing this in its current form if that's what folks want to do, though. Seems like a step in the right direction.

Crell’s picture

With our current core process, we generally commit the library on-its-own first, then use it in another patch. That's simply to keep the size of the "use it" patch manageable and reviewable. If/when we get our Composer usage sorted out this will be unnecessary but for the moment a 3/4 MB new library has to go in on its own. :-) It's also trivial enough to rip back out if we decide it's not useful after all.

moshe weitzman’s picture

I was asked to chime in here. I agree with everyone that Console should go into Drupal core. However ...

The main benefit is that the script required to get one of the remaining critical issues blocking the beta-to-beta upgrade (#2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible) would be testable, and can leverage all the formatting, input options, and argument handling that the console component provides.

If I were a D8 release manager, this would not be a compelling argument. We already have a dozen scripts doing it without a new component. One more script would not hurt. This isn't even a script that regular folks would use.

In other news, the Drush project will soon start using Console. I have invited the Drupal Console folks to join us as experts/maintainers for this area. They are considering the offer.

greg.1.anderson’s picture

Re: #33, @jmolivas and I plan on meeting again in SF to continue working on the Drush / Console integration. Haven't set a date yet, though.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Discussed with @xjm, @catch, @dries, @webchick and @effulgentsia and no one has an objections to adding this dependency. As a framework manager I'm looking forward to having more testable scripts.

There's a new version (v2.6.8) but the changes appear small so committing to unblock other work. Committed 4ad36d9 and pushed to 8.0.x. Thanks!

  • alexpott committed 4ad36d9 on 8.0.x
    Issue #2493807 by jhedstrom, cilefen: Add symfony/console component to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Status: Closed (fixed) » Needs review

Status: Needs review » Needs work

The last submitted patch, 26: add_symfony_console-2493807-26.patch, failed testing.

alexpott’s picture

Status: Needs work » Fixed

This has already been committed.

greg.1.anderson’s picture

As a side-note, Drush is making progress on making sense out of the "Dependency Hell" problem with strong site-local Drush support. See https://github.com/drush-ops/drush/pull/1595

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.