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

Check if drupal/core package exists before running scaffolding #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

T2L
Copy link

@T2L T2L commented Feb 3, 2017

No description provided.

@greg-1-anderson
Copy link
Collaborator

Thanks for this; however, I think that this PR runs the risk of doing more harm than good. Currently, if there is no core available, then you get a fatal error, and the build breaks on the bench. With this PR, this situation instead silently skips scaffolding, and you get a build that reports success, but that is incomplete and therefore will not work.

Perhaps a better change here would be to detect the fatal error, and then throw an exception with a better description of what is wrong.

@webflo
Copy link
Member

webflo commented Feb 3, 2017

Yeah i think @greg-1-anderson is right.

@T2L Are you using drupal-scaffold without drupal/core? Could you explain your idea? Thanks!

@greg-1-anderson
Copy link
Collaborator

@webflo I think that @T2L is probably using drupal-scaffold with Pantheon. Currently, our process is to include drupal/core directly, using the version provided by drupal.org; however, we pull our scaffold files from pantheon-systems/drops-8. The problem occurs when a new version of drupal/core comes out; in this instance, Composer will pull in, say, version 8.2.6 of drupal/core, but pantheon-systems/drops-8 is only available through version 8.2.5. In this instance, drupal-scaffold fails.

Perhaps Pantheon should make an exact clone of drupal/core, only for the purpose of delaying the addition of the latest tag until said tag also exists on pantheon-systems/drops-8.

@T2L
Copy link
Author

T2L commented Feb 6, 2017

Actually my wordflow was a little bit different.
I was working on a composer project template for our company. Accidently I've added dependency on drupal/drupal (instead of drupal/core). composer install has run without any glitch. Then I spotted my mistake and tried to do composer remove drupal/drupal and faced aforementioned issue. I don't think it should break composer in such way.

My vision:

  1. We clearly have a bug (uncaught exception). It must be fixed
  2. @greg-1-anderson - you are talking about "running a build", but imo composer is not a building tool, but dependency manager. However, some other building tool (maybe Phing) might use composer as a step in a bulding pipeline. Also, I do believe that it's a building tool's responsibility to verify build integrity.
  3. I understand why there is no hard dependency on drupal/core (because you want to support different flavours of Drupal core, like Drops)
  4. Seems like we need some kind of soft dependency on a Drupal core-compatible code. Obviously, this is not possible with plain composer
  5. Throwing an exception when no core-compatible code were found could work, but let's think about responsibility
  6. Ideally, we need a hard dependency on core. One possible options is to divide this package:
    1. drupal-composer/drupal-scaffold (no dependecies, as it's now)
    2. drupal-composer/drupal-scaffold-drupal (dependency on drupal-composer/drupal-scaffold and drupal/core)
    3. drupal-composer/drupal-scaffold-drops (dependency on drupal-composer/drupal-scaffold and pantheon-systems/drops-8)
    4. ... do we need more?

@derhasi
Copy link
Member

derhasi commented Feb 6, 2017

To support a different core package, we could make that configurable in Handler::getCorePackage and Handler::getDrupalCorePackage.

In addition we should improve Plugin::scaffold() to throw an explicit exception or use iO to display an error. We definitely should not fail silently there.

@T2L
Copy link
Author

T2L commented Feb 6, 2017

Huh. Code explicitly depends on drupal/core, but the package itself does not. Maybe just and a dependency drupal/core: "*"?

ADDED
Even project description states "Composer Plugin for updating the Drupal scaffold files when using drupal/core"

@greg-1-anderson
Copy link
Collaborator

The semantics of what composer is are immaterial here; if a tool of any type fails to do it's job, it should return with a non-zero exit code. An inscrutable exception is preferable to a silent failure, and a rational error message is better than an inscrutable exception.

Related issue: pantheon-systems/drops-8#168

@webflo
Copy link
Member

webflo commented Jul 4, 2018

+1 on adding an exception

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.

4 participants