Problem/Motivation

We still have lots of code and comments referring to core/update.php. This url does not exist.

Proposed resolution

Change to use the system.db_update route or the correct path.

Remaining tasks

 

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Y Instructions
Update the issue summary Y Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

Correct links in the UI

API changes

None

CommentFileSizeAuthor
#18 interdiff.txt994 bytestrzcinski.t@gmail.com
#18 core_update_php_is_now-2338759-18.patch10.74 KBtrzcinski.t@gmail.com
#15 core_update_php_is_now-2338759-15.patch9.96 KBStryKaizer
#11 core_update_php_is_now-2338759-11.patch9.96 KBStryKaizer
#7 core_update_php_is_now-2338759-7.patch9.96 KBStryKaizer
#5 core_update_php_is_now-2338759-5.patch9.74 KBStryKaizer
#3 core_update_php_is_now-2338759-3.patch9.71 KBStryKaizer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Related issues: +#2250119: Run updates in a full environment

#2250119: Run updates in a full environment was the issue that introduced the regression.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
Assigned: Unassigned » StryKaizer

Working on this

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
Status: Active » Needs review
FileSize
core_update_php_is_now-2338759-3.patch9.71 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Needs work

Can we use \Drupal::url('system.db_update'); so that we use the new routing system instead.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
Assigned: StryKaizer » Unassigned
Status: Needs work » Needs review
FileSize
core_update_php_is_now-2338759-5.patch9.74 KB

New patch using the new routing system now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Needs work
Issue tags: +Needs manual testing

Looking good - a couple of minor nits. It would also be good to do some manual testing and provide some evidence (eg html output) since we obviously don't have test coverage

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -822,7 +822,7 @@ protected function initializeRequestGlobals(Request $request) {
-        // Remove "core" directory if present, allowing install.php, update.php,
+        // Remove "core" directory if present, allowing install.php, authorize.php,
         // and others to auto-detect a base path.

    We need to wrap the comment at 80 characters 

  2. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
@@ -26,7 +26,7 @@ public function __construct(TranslationInterface $string_translation) {
+<li>To upgrade an existing installation, proceed to the <a href="!base-url/update.php">update script</a>.</li>
 <li>View your <a href="!base-url">existing site</a>.</li>
 </ul>', array(
       '!base-url' => $GLOBALS['base_url'],

    Here we should add another variable to the $this->t() args so we can use the route.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
Status: Needs work » Needs review
FileSize
core_update_php_is_now-2338759-7.patch9.96 KB

Changes added following suggestions in #6

Output below generated using patch #7

Some modules have database schema updates to install. You should run the
<a href="/update.php">database update script</a>
immediately.
</div>

<p>Regularly review and install <a href="/admin/reports/updates">available updates</a> to maintain a secure and current site. Always run the <a href="/update.php">update script</a> each time a module is updated.</p>

<p>Regularly review <a href="/admin/reports/updates">available updates</a> to maintain a secure and current site. Always run the <a href="/update.php">update script</a> each time a module is updated.</p>

<p>Regularly review available updates to maintain a secure and current site. Always run the <a href="/update.php">update script</a> each time a module is updated. Enable the Update Manager module to update and install modules and themes.</p>

<li>To upgrade an existing installation, proceed to the <a href="/update.php">update script</a>.</li>
geodaniel’s picture

geodaniel CreditAttribution: geodaniel commented

This patch works as expected for me. My only comment would be that it seems like it would be better to use the new routing system for url('admin/reports/updates') as well, for a bit of extra consistency. I.e. \Drupal::url('update.status')

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented

Hi geodaniel,

Makes sense, I will adjust those urls too.

geodaniel’s picture

geodaniel CreditAttribution: geodaniel commented

I haven't looked at any of the routing bits before, so I'm only presuming it's right to use them both in the same way. I gave it a quick try though and it seemed to work as it should. Thanks for the patches.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
FileSize
core_update_php_is_now-2338759-11.patch9.96 KB

As suggested in #8, the attached patch uses the new routing system for the "admin/reports/updates" url now.

Output with new patch for changed lines is correct:


<p>Download additional <a href="http://drupal.org/project/modules">contributed modules</a> to extend Drupal's functionality.</p><p>Regularly review and install <a href="/admin/reports/updates">available updates</a> to maintain a secure and current site. Always run the <a href="/update.php">update script</a> each time a module is updated.</p>

<p>Regularly review <a href="/admin/reports/updates">available updates</a> to maintain a secure and current site. Always run the <a href="/update.php">update script</a> each time a module is updated.</p>

<p>Regularly review available updates to maintain a secure and current site. Always run the <a href="/update.php">update script</a> each time a module is updated. Enable the Update Manager module to update and install modules and themes.</p>

Status: Needs review » Needs work

The last submitted patch, 11: core_update_php_is_now-2338759-11.patch, failed testing.

Status: Needs work » Needs review

StryKaizer queued 11: core_update_php_is_now-2338759-11.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: core_update_php_is_now-2338759-11.patch, failed testing.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
FileSize
core_update_php_is_now-2338759-15.patch9.96 KB

Updated patch to match current 8.0.x

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
Status: Needs work » Needs review

Marking as needs review

trzcinski.t@gmail.com’s picture

trzcinski.t@gmail.com CreditAttribution: trzcinski.t@gmail.com commented
Issue tags: +Amsterdam2014

Hi - thanks for the patch. Here`s what I`ve done:

I have read the patch line by line and it looks alright - I did not find any inconsistencies.

I have applied the patch as well - no problems with this. In all cases the links have been changed.

There is one thing though that I think might need some work:
in system.install file line 451 you have something like this:
$requirements['update']['description'] = t('Some modules have database schema updates to install. You should run the <a href="@update">database update script</a> immediately.', array('@update' => \Drupal::url('system.db_update')));
whereas a few lines later (line 459) it looks the same except the link:
$requirements['update']['description'] = t('Some modules have database schema updates to install. You should run the <a href="@update">database update script</a> immediately.', array('@update' => base_path() . 'update.php'));

why don`t we use the same \Drupal::url('system.db_update')))in both cases?

In addition I am not actually sure about the coding conventions - the thing about 80 chars in line - some of the comments and longer strings exceed this limits.

Other than that I think this is RTBC - I just need a clarification on those two issues.

trzcinski.t@gmail.com’s picture

trzcinski.t@gmail.com CreditAttribution: trzcinski.t@gmail.com commented
FileSize
core_update_php_is_now-2338759-18.patch10.74 KB
interdiff.txt994 bytes

I have created a patch to include the changes I found in system.install file. I attached modified patch and an interdiff.

Robin_K’s picture

Robin_K CreditAttribution: Robin_K commented
Status: Needs review » Reviewed & tested by the community

Patch applies correctly against current dev and looks clean.

The coding standards of the System module is a separate issue: #1533400: Make system module (except tests subdirectory) pass Coder Review, which makes this issue fixed with the patch in #18.
Marking RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 9315a45 and pushed to 8.0.x. Thanks!

  • alexpott committed 9315a45 on 8.0.x 
    Issue #2338759 by StryKaizer, tom_ek | alexpott: Fixed core/update.php...

Status: Fixed » Needs work

The last submitted patch, 18: core_update_php_is_now-2338759-18.patch, failed testing.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer commented
Status: Needs work » Closed (fixed)