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

Remove drupal.js from D8 root check. #1106

Merged
merged 3 commits into from
Feb 9, 2015

Conversation

stefanruijsenaars
Copy link
Contributor

No description provided.

@stefanruijsenaars
Copy link
Contributor Author

See #1105

@greg-1-anderson
Copy link
Member

The problem here is that we've weakened the Drush boostrap, making false positives more likely. Without drupal.js, none of the files we check contain the name "drupal" any longer.

I see two good ways to move forward (lacking an official way to detect the root, as mentioned in #1105):

  1. Temporarily remove drupal.js from the bootstrap test. Put it back in later, once file locations are stable. Add a comment with the new location, and a TODO to put the check back in later.
  2. Check for both the new and the old location of drupal.js. Put in a comment to remove the old location once we are ready to de-support old Drupal 8 betas (maybe after Drupal 8 stable release).

I'd slightly prefer #2.

@stefanruijsenaars
Copy link
Contributor Author

#2 seem good to me. It looks like the new location will be core/assets/js/drupal.js

@stefanruijsenaars
Copy link
Contributor Author

Updated the pull request to check for both locations.

@greg-1-anderson
Copy link
Member

Looks good. Thanks.

greg-1-anderson added a commit that referenced this pull request Feb 9, 2015
Remove drupal.js from D8 root check.
@greg-1-anderson greg-1-anderson merged commit 9f26528 into drush-ops:master Feb 9, 2015
@stefanruijsenaars
Copy link
Contributor Author

Thanks. Can we do a new Drush release so this can be installed on
qa.drupal.org?

2015-02-09 15:56 GMT+01:00 Greg Anderson notifications@github.com:

Merged #1106 #1106.


Reply to this email directly or view it on GitHub
#1106 (comment).

@greg-1-anderson
Copy link
Member

Seems reasonable. Discussion on releases is in #1123.

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