Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow psysh version 0.8.x. #2853

Closed
wants to merge 3 commits into from
Closed

Allow psysh version 0.8.x. #2853

wants to merge 3 commits into from

Conversation

greg-1-anderson
Copy link
Member

8.x branch version of #2852

@chx
Copy link
Contributor

chx commented Jul 25, 2017

This is not it. I installed http://files.drush.org/drush.phar as /usr/local/bin/drush and I got psy 0.8.0 :

➜  /tmp  ✗ drush php
Psy Shell v0.8.0 (PHP 7.1.3 — cli) by Justin Hileman
New version is available (current: v0.8.0, latest: v0.8.10)

I strace'd drush php and it's clearly reading from drush itself. https://gist.github.com/chx/39e8517e1d4f434dcc8bc29f61a9836e

Original issue at hechoendrupal/drupal-console#3448

@greg-1-anderson
Copy link
Member Author

tl;dnr: This PR does not address chx's issue, but is probably still worth merging.

Ah. If you are mixing the global Drush (whether this is the Drush .phar, or Drush installed globally by some other means), then you are encountering the dependency hell that results when combining two Composer projects together. See the trouble with two autoloaders. Your interim solution, to pin your Drupal project psysh version to the exact version that Drush is using has the weakness that it must be adjusted every time you upgrade your global Drush. The solution to that problem is to install Drush inside your Drupal project, just as you do with Drupal Console.

Global Drush is potentially vulnerable to this problem even in absence of Drupal Console, although the addition of the later project makes the problem more acute, as a) there are more packages in common, giving a larger chance for overlap, and b) Drush and Drupal Console are not tested together.

An attempt to solve the global Drush problem was started at #2787; this PR would take a lot of effort to finish. There have also been a couple attempts at collaboration, e.g. #1337, but unfortunately they did not work out.

The root cause, though, is Composer -- or, more to the point, the fact that Composer's ambitions outstrips php's capabilities, making it impossible to support per-library dependencies. Maybe things will get better in PHP 8 -- but not sure PHP will go there, as Composer builds would get a lot bigger and perhaps slower than they already are.

@chx
Copy link
Contributor

chx commented Jul 25, 2017

The real big question is, why does the include go outside of phar?

@greg-1-anderson
Copy link
Member Author

Drush must include Drupal's autoloader if it is to run any of Drupal's code. This is its purpose, so it must do this.

The better situation would be for Drupal to provide a cli in core that provided a minimal viable set of APIs for operating on Drupal core (including an API for adding commands to modules). Then, external tools that wanted to provide additional functionality could exec core's cli to do their business.

@greg-1-anderson
Copy link
Member Author

Per @damiankloip, keeping ~0.6 as our constraint. This includes the latest release (until 1.0.0).

I committed just the php lint changes to the 8.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants