Problem/Motivation

We've been making good progress in 8.x on readiness checkers. With them pretty stable at this point, now is a good time to start backporting some of that to 7.x

Proposed resolution

Put some time into building the basic building blocks so backporting all the checkers is fairy trivial.

Remaining tasks

Do it
Open Follow-ups (with a meta) to backport the rest of the checkers.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3059742-11.patch14.42 KBheddn
#11 interdiff_7-11.txt1.12 KBheddn
#7 3059742-7.patch14.07 KBheddn
#7 interdiff_4-7.txt1.11 KBheddn
#6 3059742-6.patch14.37 KBheddn
#5 3059742-5.patch14.3 KBheddn
#4 3059742-4.patch14.25 KBheddn
#4 interdiff_3-4.txt8.21 KBheddn
#3 3059742-3.patch13.85 KBheddn
#3 interdiff_2-3.txt1.13 KBheddn
#2 3059742.patch12.71 KBheddn

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Active » Needs review
StatusFileSize
new 3059742.patch12.71 KB
heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new interdiff_2-3.txt1.13 KB
new 3059742-3.patch13.85 KB

Here we add a basic test.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new interdiff_3-4.txt8.21 KB
new 3059742-4.patch14.25 KB
heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new 3059742-5.patch14.3 KB
heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new 3059742-6.patch14.37 KB
heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new interdiff_4-7.txt1.11 KB
new 3059742-7.patch14.07 KB

So, the testbot runs things in an "interesting" way when it comes to PHP_SAPI. This should make it and running tests locally happy. Fingers crossed.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

Most of this is a straight port. The lone item I think of interest:

+++ b/ReadinessCheckers/ReadinessCheckerManager.php
@@ -0,0 +1,106 @@
+  protected static function getCheckers() {
+    static::$checkers['warning'][0][] = 'PhpSapi';
+    static::$checkers['error'][0][] = 'PhpSapi';
+
+    return static::$checkers;

This is a poor-mans way to register checkers as certain priorities and categories. And let's us keep much of the D8 logic intact. Any concerns with this approach?

Don't let it confuse you that we have the same checker registered twice. I want to get this in quickly and once we add an actuall error level checker, we can remove the PhpSapi checker from here.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented

Generally this looks good, only concern I have is no we're back to Drupal 7 and variable_set() that there's potentially a lot of variable_set() calls.

  1. +++ b/ReadinessCheckers/ReadinessCheckerManager.php
@@ -0,0 +1,106 @@
+    variable_set("automatic_updates.readiness_check_results.$category", $messages);

    Is it worth comparing $messages with the current value to avoid the variable_set() if there's been no changes? 

  2. +++ b/ReadinessCheckers/ReadinessCheckerManager.php
@@ -0,0 +1,106 @@
+    variable_set('automatic_updates.readiness_check_timestamp', REQUEST_TIME);

    There's no $category on the end here - does this mean we're setting this multiple times during the same run? Ideally we'd do it in automatic_updates_run_checks() after the foreach to avoid duplicate writes.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new interdiff_7-11.txt1.12 KB
new 3059742-11.patch14.42 KB

This should address the concerns w/ variable_set. The checker manager is called from cron and an admin form so we should encapsulate as much in the manager as possible.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

  • heddn committed 9119f02 on 7.x-1.x 
    Issue #3059742 by heddn, catch: Backport base readiness checker logic to...
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Reviewed & tested by the community » Fixed

This now unleashes the possibility to easily backport the rest of the readiness checkers.

Status: Fixed » Closed (fixed)

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