Rebuild Container based on a new state flag [#1938402]

Comment	File	Size	Author
#1	container_rebuild.patch	2.27 KB	moshe weitzman

Comment #1

moshe weitzman CreditAttribution: moshe weitzman commented 12 March 2013 at 03:28

Status:

Active

» Needs review

File	Size
container_rebuild.patch	2.27 KB

As promised.

Log in or register to post comments

Comment #2

Fabianx CreditAttribution: Fabianx commented 13 March 2013 at 15:57

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -288,20 +289,33 @@ protected function initializeContainer() {
+        // Rebuild if container is dirty. Used by CLI scripts such as Drush.
+        $dirty = $this->container->get('state')->get('container.rebuild');
+        if ($dirty && $this->allowDumping) {
+          $this->container->get('state')->set('container.rebuild', FALSE);

I am not sure that the allowDumping is not only a flag that is set.

I would directly check the filesystem service like done in dumpContainer if things are writable. (as pasted in IRC - can't find the log atm.)

Log in or register to post comments

Comment #3

pounard

French

CreditAttribution: pounard commented 13 March 2013 at 16:05

Does this mean one extra SQL query per page or did I misread the issue?

State API might be useful but for a so low level stuff it seems dangerous (framework not fully bootstrapped, how am I sure the k/v store is fully init yet?) and overkill.(once again one more SQL query, and there's already a lot of gratuitous k/v access during normal runtime).

Log in or register to post comments

Comment #4

moshe weitzman CreditAttribution: moshe weitzman commented 13 March 2013 at 16:13

For default installs, it would be an extra query. Sites that want to optimize that can choose a key/value for state system. An alternate state driver might choose to do one DB query for all data in state. We don't use it too much so that seems feasible to me.

By definition this code only works once the container exists so I'm assuming that state() is available.

Can you suggest an alternate approach?

Log in or register to post comments

Comment #5

pounard

French

CreditAttribution: pounard commented 13 March 2013 at 16:17

No, sadly I can't, I just wanted to point out that out. Using a service for rebuilding the service container seemed a bit wonky to me.

Log in or register to post comments

Comment #6

Damien Tournoud CreditAttribution: Damien Tournoud commented 13 March 2013 at 16:29

Running command line tools under a different user then the web server is asking for trouble sooner rather then later.

I don't see how this is different then any other permission issues you are going to bump into in that case. It feels like we are trying to workaround a server/environment configuration issue here.

Log in or register to post comments

Comment #7

pounard

French

CreditAttribution: pounard commented 13 March 2013 at 16:33

I fully agree with #6, it's not really Drupal's code's cross to bear.

Log in or register to post comments

Comment #8

moshe weitzman CreditAttribution: moshe weitzman commented 14 March 2013 at 19:04

Status:

Needs review

» Postponed

For the moment, Drush is going to pursue a different strategy which is to write a patch to let folks optionally open up their MTime directories by making a settings.php modification. We'll open this back up if that effort is not fruitful. No issue to link to just yet.