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

#3123 - Global D8 container is not available during container rebuild. #3124

Merged
merged 8 commits into from
Nov 4, 2017

Conversation

kevin-dutra
Copy link
Contributor

Here's the suggested change to fix #3123

@greg-1-anderson
Copy link
Member

This looks compelling, but I don't have the background to know if this is the right thing vis-a-vis Drupal's expectations & your comments in #3123. Maybe @weitzman knows better.

Do you think that perhaps you could provide a test that passes with this PR, but fails without it?

@weitzman
Copy link
Member

weitzman commented Nov 1, 2017

Unfortunately, I dont know.

How fast o slow slow is this new code? Trying to evaluate any perf impact.

@kevin-dutra
Copy link
Contributor Author

kevin-dutra commented Nov 1, 2017

In terms of performance, the added code adds very little overhead, and it only runs when the container is already being rebuilt, which in theory should not be too often.

As for a test, I'll have to think about that a little. It would be tricky because we'd need to somehow get the container built within the normal context of a Drupal request so that the Drush add-ons were not there. That way the next Drush command that gets run would trigger the container rebuild to add the Drush pieces in (thereby executing the new code).

@kevin-dutra
Copy link
Contributor Author

Alrighty, I managed to craft a test that passes with the change and fails without it.

@jhedstrom
Copy link
Member

Looks like this needs to merge/rebase the latest master.

This is the related Rules issue: https://www.drupal.org/node/2816033

Copy link
Member

@weitzman weitzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a link to this issue in the test and in the code. My future self wont have a clue what this is about without that. Thanks.

$root = $this->webroot();
$options = array(
'root' => $root,
'uri' => key($sites),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now use $this->getUri() instead.

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.

Global D8 container is not available when rebuilding the container
4 participants