Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In #3052541-6: Checker: File owner and php script user are different we came up with an mechanism to distinguish how the checker is being run. Let's continue working on that.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#9 | 3053712-9.patch | 3 KB | heddn |
Comments
Comment #2
heddnI thought I uploaded the patch earlier. Here it is now.
Comment #3
eiriksmThere are quite a few coding standard errors in the patch, for example missing function comments and missing new lines. Just pasting a couple others I also found after a brief look through
In addition. Even if we set version to php 7, can we really rely on scalar type hints at this point? If we want to have this in core?
missing forward slash
Should be {@inheritdoc}
Should be one space after function keyword
Comment #4
heddnLet's leave this off as long as possible. I'd like to backport as much as possible to D7, so unless we have a very compelling reasons to use PHP 7+, I'd rather remove this version constraint.
I really don't like how the context addition makes the interfaces bleed so much. Can we add a context service, as done here in this patch. But then set the context in the checker manager? And on checkers where it matters, inject the context service to obtain the context?
Comment #5
heddnI was starting to look into what it would take to implement my suggestions above. And went full circle. What are we trying to solve here?
$this->state->get('system.cron_last')
and warn if it is over 60 minutes.Parting thoughts; we need to support manually running updates (via a button) and applying updates (via cron). And we need to be able to turn off cron application of updates so manual is the only option.
Ok, that was a lot of thinking. Does any of this line up with anyone else's thoughts?
Comment #6
heddnI've held off looking into this much more since I posted #5 because I wasn't sure if what I stated had general consensus. Are those bullet points something to consider moving forward?
Comment #7
heddnComment #8
heddnFor cron warning/error, I propose that we adjust core's existing warnings and errors. It already posts errors/warnings when cron hasn't run recently in system.install. Using https://www.drupal.org/docs/8/api/configuration-api/configuration-overri..., we could check existing levels and warn if cron doesn't run at least every X and error if it hasn't run within Y.
Since core currently defaults to 3 hours for running cron, I don't think we want to warn differently on X. The patch in #5 warns on 60 minutes. And maybe we want to double that for Y and error if hasn't run for 6 hours?
That's run time. We should also error if cron is setup to run anything but 3 hours or Never.
Lastly, let's also add a warning and messaging if the PHP_SAPI changes between checker runs.
Going to start work on the PHP_SAPI checker first.
Comment #9
heddnSimple enough. No interdiff.
Comment #10
heddnI've opened #3058250: Cron frequency for the cron execution checking.
Comment #11
catchLooks great.
Comment #14
heddn