From #1872522-84: Compiled data in PHP storage is cleared too late in drupal_flush_all_caches():

I'm also not sure about core/rebuild.php and core/scripts/rebuild_token_calculator.sh. We've got existing issues to add something like that (although those were originally due to the D7 registry) and I'm sure there's discussion on those that isn't reflected here. For example why a cli token generator and not a flag in $settings similar to update_free_access? The latter would allow people with no cli access to rebuild for example, whereas this makes things command line only - but if it's command line only then why not directly rebuild in the shell script anyway?

I have separated out the rebuild files to here but I will not have the time to work on the feedback above.

Files: 
CommentFileSizeAuthor
#23 2097189-22.patch5.66 KBsun
PASSED: [[SimpleTest]]: [MySQL] 59,228 pass(es).
[ View ]
#22 interdiff-2097189-22.txt2.87 KBdamiankloip
#22 2097189-22.patch6.29 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,282 pass(es).
[ View ]
#20 interdiff.txt2.67 KBsun
#20 drupal8.rebuild.20.patch5.96 KBsun
PASSED: [[SimpleTest]]: [MySQL] 59,155 pass(es).
[ View ]
#16 interdiff-2097189-16.txt754 bytesdamiankloip
#16 2097189-16.patch5 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,012 pass(es).
[ View ]
#1 drupal8.base-system.2097189-1.patch2.02 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/rebuild.php.
[ View ]
#1 interdiff.txt1.73 KBAlbert Volkman
reb.patch2.03 KBchx
PASSED: [[SimpleTest]]: [MySQL] 59,099 pass(es).
[ View ]

Comments

Albert Volkman’s picture

StatusFileSize
new1.73 KB
new2.02 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/rebuild.php.
[ View ]

Haven't addressed above issues, but updated the patch to replace drupal_hmac_base64() with Crypt::hmacBase64().

damiankloip’s picture

Issue summary:View changes
Status:Active» Needs review
StatusFileSize
new2.23 KB
FAILED: [[SimpleTest]]: [MySQL] 59,053 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.78 KB

How about something like this?

damiankloip’s picture

StatusFileSize
new2.73 KB
FAILED: [[SimpleTest]]: [MySQL] 53,161 pass(es), 4,007 fail(s), and 1,015 exception(s).
[ View ]

Sorry, forgot to add the .htaccess change to the actual patch.

catch’s picture

Priority:Critical» Major

I think we should add this but we needed the same thing for Drupal 7 and survived OK with drush rr in the end, so don't think it counts as critical.

Status:Needs review» Needs work

The last submitted patch, 3: 2097189-3.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review

3: 2097189-3.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1: drupal8.base-system.2097189-1.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review

Just settings as needs review, for...reviews. The bot is being funny atm, and we have no tests for this anyway.

jibran’s picture

Patch seems fine to me but I think we should add a rebuild link here

+++ b/index.php
@@ -9,4 +9,10 @@
+  print 'If you have just changed code (for example deployed a new module or moved an existing one) read http://drupal.org/documentation/rebuild';
olli’s picture

Is it possible to write a test for this script?

  1. +++ b/core/rebuild.php
    @@ -0,0 +1,43 @@
    +if (settings()->get('rebuild_access', FALSE) ||

    Should we add a few lines to settings.php about this new setting and maybe a warning on status page if it is enabled?

  2. +++ b/core/rebuild.php
    @@ -0,0 +1,43 @@
    +  $GLOBALS['conf']['system.performance']['cache.page.use_internal'] = FALSE;

    Is the purpose of this line to disable page cache? If so, should we use drupal_page_is_cacheable(FALSE)?

  3. +++ b/core/scripts/rebuild_token_calculator.sh
    @@ -0,0 +1,24 @@
    +if (PHP_SAPI !== 'cli') {

    Why not drupal_is_cli()?

damiankloip’s picture

StatusFileSize
new4.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,053 pass(es).
[ View ]
new2.43 KB

Thanks both for the reviews.

#9, I am not against adding text to the exception message, I would personally prefer to just update the content on http://drupal.org/documentation/rebuild with these instructions, as having these on this page, or even a link to rebuild.php here would be strange IMO.

#10, good point, The last patch was a bit rough :) (I just fixed up the initial patch). Added the setting to default.settings.php with some docs, added a check to system_requirements, and others.

chx’s picture

Why don't we check the setting? If access is allowed, present the link to rebuild.php (but don't skip the docs) otherwise only link the docs. We can check the setting even if Drupal is in a really sorry state, it's really really standalone.

jibran’s picture

I think #12 makes sense form DX pov.

damiankloip’s picture

Yes, that seems like a good compromise. Doing that...

damiankloip’s picture

StatusFileSize
new4.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,106 pass(es).
[ View ]
new687 bytes

Sorry about the delay, this one went off the radar. How about something like this?

damiankloip’s picture

StatusFileSize
new5 KB
PASSED: [[SimpleTest]]: [MySQL] 59,012 pass(es).
[ View ]
new754 bytes

ok, let's try that again (Thanks chx!). chx also suggested just making them links, makes sense to me. Interdiff showing new changes.

chx’s picture

Status:Needs review» Reviewed & tested by the community

I like the token script because you might have a dev version of the site and so you are able to generate the token locally -- shell access on production is never guaranteed.

webchick’s picture

How can I trigger this condition in order to test that the script works?

sun’s picture

Could we move the main functionality of the rebuild.php script into a drupal_rebuild() function in bootstrap.inc, so that e.g. Drush can re-use the facility without having to duplicate it?

sun’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new5.96 KB
PASSED: [[SimpleTest]]: [MySQL] 59,155 pass(es).
[ View ]
new2.67 KB

Here's what I mean -- no functional changes.

damiankloip’s picture

I'm not sure why a function like that should live in bootstrap.inc?

damiankloip’s picture

StatusFileSize
new6.29 KB
PASSED: [[SimpleTest]]: [MySQL] 59,282 pass(es).
[ View ]
new2.87 KB

Ok, as there has been radio silence, how about we compromise here and add this to utility.inc instead? It is a utility function, and it's not in bootstrap then.

sun’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new5.66 KB
PASSED: [[SimpleTest]]: [MySQL] 59,228 pass(es).
[ View ]

Works for me. As long as there is a helper function that can be invoked through other mechanisms, I'm happy.

Same patch as the previous, but sans the extraneous use statements in bootstrap.inc that are obsolete now.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 23: 2097189-22.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review

23: 2097189-22.patch queued for re-testing.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC then.

damiankloip’s picture

Yep, +1 on #23 removing the use statements I forgot to remove. An interdiff would have been nice though... :)

catch’s picture

Title:Add a rebuild script» Change notice: Add a rebuild script
Status:Reviewed & tested by the community» Active

Committed/pushed to 8.x, thanks! Could use a change notice for the new feature.

damiankloip’s picture

Status:Active» Needs review
damiankloip’s picture

Cottser’s picture

Thanks! I revised the change notice a bit to clarify there are two methods of running the rebuild script.

We can remove the php from the instructions for running the script once #2153741: Add executable permission to rebuild_token_calculator.sh script is taken care of.

jibran’s picture

Title:Change notice: Add a rebuild script» Add a rebuild script
Status:Needs review» Fixed

Great work. Thanks everyone.

jhodgdon’s picture

This needs a follow-up so that when Drupal doesn't bootstrap, it points you to this rebuild script instead of that stupid link in index.php to drupal.org (which will really not help if you're for instance on an airplain).

  $message = 'If you have just changed code (for example deployed a new module or moved an existing one) read <a href="http://drupal.org/documentation/rebuild">http://drupal.org/documentation/rebuild</a>';
  if (settings()->get('rebuild_access', FALSE)) {
    $rebuild_path = $GLOBALS['base_url'] . '/rebuild.php';
    $message .= " or run the <a href="$rebuild_path">rebuild script</a>";
  }

This doesn't help if you didn't already have the rebuild access set to TRUE. It should tell you to set it to TRUE, like it does if you try to run update.php.

And I also noticed if I tried to run the rebuild script without it being set to TRUE it failed with no explanation, if the site was hosed in the first place -- I just got back to the unhelpful index.php message.

So I don't think this is really finished. I'd like to reopen but a follow-up would be OK too.

jhodgdon’s picture

The change notice is also not showing up on
https://drupal.org/list-changes/drupal/

Why is that????? Oh, the Project wasn't set. I think it needs to be a required field. I'll file a separate issue.

damiankloip’s picture

#34, The purpose of this issue is finished: "Add a rebuild script" - added.

Feel free to open a followup and propose whatever changes there. I've said before, we seriously need to stop repurposing issues.

And I also noticed if I tried to run the rebuild script without it being set to TRUE it failed with no explanation, if the site was hosed in the first place -- I just got back to the unhelpful index.php message.

What else do you propose happens here? If your site is not functional, there is not much we can do here. That exception message is IT. We intentionally just redirect this script back to the site. Adding to the exception message is certainly an option.

Also, update.php has a full page with markup (basically). So might not be quite the same, as update is using drupal to render that. Some of which may not be possible if the dumped container is borked.

jhodgdon’s picture

Yes, I know I know but the patch that was committed had bugs in it... Anyway, I opened a separate issue:
#2156287: Followup to new rebuild script - index.php messaging is incorrect/incomplete
as requested.

damiankloip’s picture

Not sure how that's a bug, but ok. most of what you're talking about I'd based on opinion.

Also, we just added to the message. We didn't add the d.o link.

moshe weitzman’s picture

Follow-up - Might be worth adding a settings.php array of PHPStorage bins that should be cleared. That way a site could clear more than twig and service_container. The script currently privileges those systems.

damiankloip’s picture

Possibly, Not sure most implementations using any php storage will be able to cripple a site like an out of date service container though?

sun’s picture

The rebuild script failed to rebuild for me today and ended in a fatal error instead; follow-up issue:

#2159459: Rebuild script triggers errors in error handler, fails to rebuild container

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

YesCT’s picture