As part of phase 2 of #2940731: Automatic Updates Initiative overview and roadmap we need Readiness Checks for automatic updates.
These checks would ensure that a site is ready to have automatic updates applied.
The Automatic Updates contrib module has a readiness check system built on tagged services.
These Readiness checks are particular to the process of Automatic Updates in the contrib module and should also work for the proposed Composer based updates for Drupal core.
Because Drupal core will introduce the Automatic Updates in an experimental module and Readiness Checks will be checking for this yet to be implemented functionality it does not make sense to add the Readiness Checks directly to the existing Update module. For more info on this see this comment on the roadmap issue.
Add the Readiness check infrastructure and 1 Readiness Check service from the contrib module to a new experimental module.
There also the problem where there is not a way to tell which messages come from which checkers
\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManager::run() does not throw an exception if it is run for a category that has no checkers or ensure it does not run in this case. Services can be altered so we don't know for sure what the existing checkers will be or which category they will be in.ReadinessCheckerManager that test deleting the key/value results works as expected, refreshing, providing checker services dynamicallyQuestions
\Drupal\automatic_updates\ReadinessChecker\DiskSpace::MINIMUM_DISK_SPACE?For instance the DiskSpace check might return an "error" message "Your disk space is too low" and a "warning" message "Your free disk space is nearing the limit for automatic updates".
There will be message on the Status Report page that tells you if your site is ready for Automatic Updates
TBD
None
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #68 | 3162655-68.patch | 45.14 KB | tedbow |
| #68 | interdiff-66-68.txt | 23.6 KB | tedbow |
| #66 | 3162655-66.patch | 45.97 KB | tedbow |
| #66 | interdiff-61-66.txt | 15.17 KB | tedbow |
| #61 | 3162655-61.patch | 50 KB | tedbow |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
tedbowHere is the initial port from the contrib module. I just copied over files needed and tried to remove all part not necessary for the Readiness Checks.
I have not given this a review. I will do that now.
Comment #14
tedbowFixing the test and some nits.
Also attempting to credit everyone that was credited on the #3043521: [META] Update readiness checks for autoupdate (pre-flight check) and other issues as this code was copied from there.
Comment #15
johnwebdev commentedShould maybe return an empty array here?
Why not have this directly in the hook_requirements?
/s/to for/for
Maybe ::getResults() could be extended to accept bitwise operators. getResults(ReadinessCheckerManagerInterface::ERROR | ReadinessCheckerManagerInterface::Warning).
Aha. That makes the suggestion above harder, but that could be solved by returning getResults() returning a Result object instead of an array.
@ should not be used within HTML attributes.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Rend...
Nit: Missing a blankspace between.
Is this the right permission?
Should use ":" here.
When do you want to disable readiness checks?
Can be marked internal.
Maybe: A redirect to the automatic updates settings page.
Maybe a comment here to explain that we're flattening the array?
Nit: Missing a blank space.
ignored_paths is not present in the schema.
"It is recommended to ignore paths to contrib extensions"... why?
Why does this checker extend the Filesystem? Couldn't it just get it as a dependency through the constructor?
Oh... nevermind. Maybe we should suffix checker classes with "Checker"
What is this based on?
Do we really need the "Manager" suffix. I think "ReadinessChecker" is fine.
Wondering if a checker should be able to return messages for multiple categories instead? i.e. The DiskSpace checker could then have a warning "Your disk space is close to being too low, etc."
Is "0" low priority, and a higher number is higher priority?
Nit: Maybe this comment should be in the class doc comment instead.
Comment #16
tedbow@johnwebdev thanks for the review!
administer site configuration- This is because the contrib module had other types of requirements it was checking and had multiple function calls here. But the PSA functionality will likely go directly into the Update module so it will not be needed here. removing _automatic_updates_checker_requirements()
- fixed
- I could be missing something but these are strings would this work?
- Leaving for now. I think returning the strings is more understandable.
- Fixed and changed to ":url" because is used more in core than ":link". Also fixed other occurences.
- fixed
- Adding a todo in the patch to check on this. I want to see why this was chosen because it is used multiple places in the contrib module.
- fixed
- Eventually the module will have the ability to actually do the automatic updates. You may want to turn off this functionality but leave the Readiness checks on. But you may want to turn them too if you know there is issue with the site and don't want to see the message again and again.
- fixed
- fixed
- added a comment but moved the array_merge into the if since we don't need it again.
- fixed
- This is for another check in the contrib module that we will add later. Removing
- Removed but the initial support will on be for core
- I am inclined to not add "checker" because it is redundant with the namespace but I could be persuaded.
- Add this as a question in the summary. The contrib module used a different patch based system so we would probably need a higher number
- Well we already have
- Good question. I have put this as a question in the summary to get feedback from original contrib authors as to why it was architected this way.
- Looks that way because of
- yep fixed.
ReadinessCheckerInterfacewhich each check will implement. This manages all of those checkersI could definitely see 1 checker has messages in both categories.
krsort($checkers);I expanded the comment.
Comment #17
tedbowThis patch changes the machine name for the module from
automatic_updatestoauto_updatesThis is prevent composer problems for sites that already have contrib module used as
drupal/automatic_updates.I chatted with @xjm, @gabesullice, and @Wim Leers about this because they were involved with adding
jsonapito core which also already had a contrib module with the same machine name. They recommended using a different machine name.Comment #19
tedbowFixing the test fails and working on functional tests.
I think one problem currently is
Comment #20
heddnRe #20: we could add cache tags and cache invalidation in a more traditional manner.
Re: MINIMUM_DISK_SPACE
We'll have to do some calculating. But anything less then 100MB seems valid to me. Empirical testing will help.
Re: checkers returning multiple categories
These checkers are really simple. I don't see any benefit to having a single hunk of code do 2 things. It follows the spirit of single-responsibility principle (SRP) much better that way.
Comment #21
heddnThis is the same permission we use in update.module. I'd suggest we keep the same and (if necessary) open a follow-up to see if we want to change that permission. Because if we change it here, we'd want to change it there too. But I'm not advocating we pick a new permission because an update to the site's code is fully in the wheelhouse of a site's administrator.
I suggest we use the batch API for this instead of a controller callback. It would scale better. It was easier/faster to do it in a controller, but I wished we'd done batch.
I think a few of these are not needed until later. The variables aren't in use.
spelling of todo.
The amount of documentation for the sorting is on par with the rest of these types of things in core. However, adding a comment on how we weight things is probably a good idea so its more clear if/when folks start playing around with weights.
Comment #22
tedbow@heddn thanks for the review!
re #20
I also added an item in the remaining tasks to determine a larger minimum value would ever cause random failures on Drupal Ci.
I guess I am not sure if this is always 2 things. With the disk space example I could see it as 1 check for how much disk space is available. But whether it is "warning" or "error" depends on how much space is left.
\Drupal\auto_updates\ReadinessChecker\DiskSpace::diskSpaceCheck()returns multiple messages. We want each of those messages to be a "warning" if there is > 200 && < 100mb left but an "error" if there is < 100 mb left. I could see this case because you want to warn site admin if it is getting close to <100mb but you don't want stop an update unless it is less than 100mb(which you need an "error" to do). Otherwise would have to do 2 Readiness checkers to achieve this and it actually becomes more complicated.Leaving for now for more opinions and thought. I am still undecided on this.
re #21
/admin/reports/statuswhich allow you to see these same messages.Well at least to know about the readiness to updates. The system module already has the permission
administer software updateswhich the current contrib module and the core Update module use for the actual updates.other changes since #17
'
This service is not used in this patch.
We don't use these yet. Removing.
Found this because new test failed on this because the module is dependent on the Update module yet.
Comment #24
tedbowhook_cronimplementation. adding that and a test for it.Comment #26
tedbowThis should use
\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManagerInterface::WARNINGBack to #20 about the checkers being restricted to providing messages in just 1 category.
Since
\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManagerInterface::run()is called with a$categoryargument at least the API provides a way to run checkers for just 1 category. But we record a single timestamp. So if it was called only for "warning" then we would have no way of knowing this after the fact thatreadiness_check_timestampwas just when "warning" checkers were run.We could change
\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManagerInterface::run()to not have an argument and just run both(all) categories. This way thereadiness_check_timestampwould actually be the last time all the checkers were run.I have local patch for this but not posting because of the next point
\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManagerInterface::run()is called with a$categoryargument is because the in the contrib module only "error" category is run before anInPlaceUpdate.We could just run all checks and only not update if there are error messages but maybe the contrib module was made this way to avoid doing extra checks before an
InPlaceUpdate?If we leave things the way they are in the contrib module with InPlaceUpdate only running "error" checks then if you come status report after a failed InPlaceUpdate then
$last_check_timestampcould be very recent but the "warnings" checkers might not have been run in hours. So this would mean the checker results would be display with the "errors" being very recent results and the "warning" results being old.This makes me think we should always run all checkers. So I will add a patch for that.
Comment #28
tedbowI don't think we actually need
auto_updates.cron_last_checkDo we actually care when cron last ran the checks or just when they were last run? If a user just ran the checks and then cron runs 5 minutes after do we actually want to run them again.
I think the intent here if the checks haven't been run in an hour run them. We can do that by relying on
\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManager::timestamp().hook_install()to run readiness checks on module install. Is there any reason we shouldn't be doing this? All the checkers in the contrib module take 200 msecs for me to runhook_modules_installed()to run readiness checks again if the available checkers have changed.Comment #29
tedbowWe are calling the manager a "checker" and but also a list of checkers "checkers". We should name this
checkerManagerto be clear.Comment #30
tedbowI refactored
auto_updates_requirements()because writing the test it was confusing to see that actual behavior is.Longer than this snippet but with the current
if/elselogic$requirements['auto_updates_readiness']['description']could be set and then re-set 2 more times.IMHO I think the refactor is more readable.
I expanded the test coverage also and the tests pass before and after refactor.
In 1 case we use "Readiness checks were last run @time ago" and another "Last run @time ago"
I think these should be the same. Changed to the longer
change this test to assert the complete text on the status report not what is there and not. It is much simpler.
Comment #31
tedbowThis service should be suffixed with "_manager"
Currently if the checkers have been disabled and user clicks an old link to run the checkers or if they enter the address in manually this message will be displayed.
We should had a
_custom_accesscheck to the route so that it is only accessible if the checkers are enabled.Changing to more common pattern I see in core "Defines an interface for * services."
Adding test coverage for disabling readiness checks.
The item should not be status report and readiness checks should not run on cron.
We should also assert the actual message texts.
Comment #32
heddnThis is coming along very well. Just a few small nits and questions.
Does it make sense to add a helper method that calculates this in a single place? Date math is always complicated and centralizing it would make it easier to test.
Maybe call this
clearStaleResults?+1
Do we want to open a follow-up for this?
This all works in D8 and tarball when we don't support a relocated docroot for auto updates. But in D9 and composer, we need to support this. Is this a good time to introduce
webflo/drupal-finder. I'd also support punting that to a follow-up and just getting this in AS-IS.Needs an issue opened or we should address here, no?
Comment #33
tedbow@heddn thank again for the review
Now we are not using
LAST_CHECKED_WARNINGoutside of the manager class so making thatprivate constgetRootPath()to handle if it not set.Created #3166435: Support alternate 'vendor' folder locations in auto_updates module as a follow up.
Comment #34
heddnAssuming this comes back green, I'm +1 on RTBC. I'm a big fan of commit often and early. This has had a lot of good review, let's land it and iterate on any potential improvements in follow-ups.
Comment #35
dwwThis mostly looks great. I love how thorough the test coverage is! Bravo.
As usual, I have a bunch of minor cosmetic things, and a handful of questions of substance.
General point (flagged below in detail): since this is all new code in 9.1.x only, just about everything could use return types now.
Also general question on terminology with "checker" vs. "check". I didn't flag it everywhere.
Anyway, here's the gory list. ;)
Thanks/sorry,
-Derek
Update: Most of this is fixed in comment #38.
Shouldn't 'auto_updates' be after 'Core'?
'readiness_check_manager'?
"checker manager" seems clumsy.
Should this be warning instead of error?
@readiness_checks vs. :readiness_checks -- why different in the two places?
Do we need the local var? Why not just:
\Drupal::service('auto_updates.readiness_checker_manager')->run();?We should mention this in the .info.yml file, too.
Core before auto_updates
Could perhaps tweak these with a UX review, but not needed before commit. However, we could at least be consistent about ending with : vs. . for now.
Definitely don't need these two lines again. We already have a $checker_manager here.
... ? ;)
Shall these match?
"Run the readiness checks.", right? I don't get what "checkers" means here.
In this case, should it really skip down to the section on the readiness_checks, or should it just send you to the top of that page?
Since this settings form has a single checkbox with a description, why are we wrapping it in a details container? I thought the UX guidelines say if a form only has 1 thing, don't wrap it inside a fieldset/details.
FileSystemBase
Why not just put the body of diskSpaceCheck() directly into doCheck() itself?
I believe this wants a return type now.
Seems weird that we have a single value for MINIMUM_DISK_SPACE that applies to each partition / filesystem separately, yet temp space doesn't care where it lives. So if I have /tmp $root and $vendor each on their own partitions, in theory the MINIMUM for each should be less than if all 3 are on the same partition and /tmp is competing for space with $root an $vendor. This logic doesn't totally make sense.
Seems either we want separate minimums for each of these 3 things, and we want to enforce them such that if any of them are on the same partition, that disk must have enough space for both/all...
Return type.
If you're gonna implode(), why not go all the way? ;)
!file_exists(implode(DIRECTORY_SEPARATOR, [$this->getRootPath(), 'core', 'core.api.php']))If we check all the way for core/core.api.php as a file in root, do we want to check for a specific file from vendor?
If not, why not just check for
is_dir(getRootPath() . DIR_SEP . 'core')?Before this is stable, I'd love a UX review of all these messages. ;) E.g. this one seems like it wants to link to a handbook page somewhere. Seems very non-actionable to non-technical users.
Performs
Gets
Return type.
Return type
Determines if the root and vendor directories are on the same logical disk.
Runs a check.
The comment above for doCheck() seems more clear:
"An array of translatable strings if any checks fail."
Return type.
Is that really what this is still for?
A) "Time (in seconds) since the last check after which we generate a warning." (or something).
B) Probably it's obvious, but maybe "Defaults to 1 day." and/or use 60 * 60 * 24?
Return type -- but what's the correct syntax for that when a function returns $this? Do we just use the raw class name?
return types
' not "
ordered by
Ids delimited by '::'.
instead of ! and >, why not just <= ?
return getRequestTime() <= timestamp() + last_checked
?
Every method on this interface could define its return type.
...of the check. Defaults to 'warning'.
s/If not provided the default priority is 0./Defaults to 0./
s/Run/Runs/
s/array readiness/array of readiness/
s/is is/is/
Gets the results of the most recent run.
Translatable messages if any checks fail, otherwise an empty array.
(or something)
Gets the timestamp of the most recent run.
The timestamp of the most recently completed run, or NULL if...
getTimestamp() ?
Determines if readiness checks are enabled.
Gets
TRUE if the results were cleared, otherwise FALSE.
s/True/TRUE/
s/false/FALSE/
A test readiness checker.
Why bool| ?
I guess we don't want auto_updates* installed until after cron is disabled? Maybe a comment about why not just use self::$modules for these?
A) s/the the/the/
B) add comma after "updates"
Would be helpful to pass the user to this so an assertion failure could tell you which user it was using at the time.
' vs. "
' not "
why '>' in here?
Could also assert the response code.
Should we use something not actively being moved towards deprecation and removal from core? ;)
Originally flagged this since I didn't understand 125 vs. "1 hour" in this comment. Re-reading it, I see the comment threw me off. Maybe:
"// Tests that running cron again over an hour after the previous cron run will run them again."
(or something).
s/an insanely/a very/
(more inclusive for folks with mental health issues -- some people take offense at such language).
Comment #36
dwwp.s. I just tried and https://www.drupal.org/docs/8/update/auto-updates is a 404. Tagging for some doc updates to have a page at that location, or to fix the links if that page already exists somewhere else.
Thanks!
-Derek
Comment #37
dwwI'll fix most of this now, stay tuned.
Comment #38
dwwThis fixes everything from #35 except:
2, 3, 4, 12, 13, 14, 16, 18, 21, 22, 31, 55, 56, 58, 61, 63
Fxed but worth confirming the fix:
33, 64
Now I don't feel so sorry for the 65 point review. ;)
Cheers,
-Derek
Comment #39
ressaGreat work @tedbow and @dww, thanks! Just one comment:
Technically, one megabyte is written as
1 MB,so perhaps100mbshould be written as100 MB(with space) in stead?https://web.stanford.edu/class/cs101/bits-gigabytes.html
https://www.npl.co.uk/si-units
Comment #40
ressaA patch for the
100 MBcomment above.Comment #41
tedbow@dww thanks for the review!
re #35
'
@varline IDE are able to find the call to the method. This makes automated refactoring easier for method name changes and also for method removals. It also just makes it easier to find all the times a method is called.changing back for now.
The naming seems fine with me but we could change it all places if others think we should call the classes "Readiness Checks"
but should we link to some page provided by the module explain this. The module is hidden by default for now so I don't think this is likely but still we don't want anyone to think they will get security updates now.
leaving for now.
diskSpaceCheck()could have a more descriptive comment? But I think if we make the class comment better then "doCheck()" say enough.fixiing.
The current logic was based the contrib workflow which explicitly copies files into
\Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory()but it is not Composer based.vendor/autoload.phpThat will always be there correct?
\Drupal\Tests\user\Traits\UserCreationTrait::createUser()returns but there is no need for it. Fixedforeachloops I put in to loop through the 2 users. Then ifassertReadinessReportMatches()fails will be able to tell the user from the call line number in the error.fixed
helpre #40. Thanks, looks good!
UPDATE: whoops made all that a
<ul>fixed.Comment #43
tedbowUpdating remaining tasks
Comment #44
tedbowFixing the coding standard issue.
#41 was just a unrelated random Javascript test failure. I miss those, good times 😜
Comment #45
dwwRe: #41: Thanks, @tedbow! All makes sense. +1 to your changes / answers there.
vendor/autoload.php. Makes sense. However -1 to usingimplode()for a single delimiter. ;) Fixed here.Everything else looks great. Interdiff is happy and addresses all the points.
So it seems we're very close, and down to these 2 points. Looks like many of the remaining tasks are already done. Can you confirm and cross off everything that's complete? Seems like points 1, 2, 3 and 8 are all done, right? Do we need a follow-up for 5? Are the "questions" there still relevant?
Thanks!
-Derek
p.s. interdiff is a bit confused since your Git is renaming a big_pipe_test module and mine isn't. So here's also the local commit I made that's all I really changed from #44.
Comment #46
dwwWhoops, stray space snuck in. ;)
Comment #47
dwwRe: #36: Oh, I see. It's https://www.drupal.org/docs/8/update/auto-updates (link in the patch) vs. https://www.drupal.org/docs/8/update/automatic-updates (actual page).
So yeah, definitely NW. Not sure how to best approach it.
Cheers,
-Derek
Comment #48
tedbowI also bumped up the minimum disk space to 1 GB. On #3166416 it was pointed out that even shared hosting plans have 50 GB or space. It is probably better to start high.
I also changed the check to so that if the root and vendor are on 2 different logical disks it will check for minimum space/2 for each.
I think the services should not have the "category" tag but return a nested array like
\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManagerInterface::run()does.My reasoning for this is because the disk space checker for example might want to return a "warning" if you have < 500 MB but an error if you have < 1 GB. To do this with the current architecture you would have to make DiskSpaceWarning and DiskSpaceError.
@heddn has said we should stick with the current method. Other opinions?
I think if this patch passes we can cross that off. I ping @mixologic about this to see if he thinks it would be problem with DrupalCI
Comment #49
tedbowConfirmed with @mixologic that #48.4 won't be a problem
Comment #50
tedbowSome nits, and comments
Comment #52
voleger#33 Added comment for #3166435: Support alternate 'vendor' folder locations in auto_updates module. Maybe we can try to update the patch as the module not commited jet.
Comment #53
phenaproximaPartial review without any manual testing...
😳Hidden and experimental...
Nit: Maybe $readiness_check_url would be a more accurate name for this variable.
This seems a bit severe to me. Maybe REQUIREMENT_WARNING would be more appropriate?
IMHO, the word "manually" implies that it'll take a bunch of work on the part of the user. How about replacing "manually" with "now" instead? Also, a supernit:
:urlwould be a more accurate placeholder for the URL.So...this is not actionable. We're treating it as an error, but there's nothing for the user to do or click on to correct the problem. That's not super great :)
These feel like they should just be two different methods of ReadinessCheckerManagerInterface:
getErrors()andgetWarnings(). Is there a reason that interface constants are preferable?Nit: $is_syncing is not used.
Supernit: technically there is no need for the $checker_manager variable, but I do understand why it's there :)
Wrapping all of this in a check for the
administer site configurationpermission feels a bit wobbly. Maybe we could at least add a comment here explaining why we're using that specific permission to cordon all of this stuff off?Nit: You can just use $route_match->getRouteName() here.
$modules should be type hinted as an array.
But more to the point -- do we maybe also want to run the checks during hook_modules_uninstalled()?
Not a huge deal, but IMHO, "severity" would be a better term than "category" to describe the nature of the check; it would be keeping with our existing concepts and language in hook_requirements().
Given that the whole point of this module is to do readiness checks, I kinda wonder if there's any real point in making this a configuration flag. If people don't want to see readiness checks, why couldn't they just uninstall the module wholesale?
Nit: We should use $this->setStringTranslation() to keep the abstraction non-leaky. :)
Why no return type hint on this method?
Any reason why we shouldn't mark this class as internal?
Any reason not to make these protected or private?
If stat() has an error, PHP's documentation says it will return FALSE. In that case, this method will report something that isn't necessarily true -- it'll say that the root and vendor aren't on the same logical disk, when in fact they might be, but we're just not aware of it due to an error condition.
So, IMHO, the right thing to do here is to throw an exception, or at least log a warning to watchdog, if stat() returns FALSE for either $root_statistics or $vendor_statistics. That'll leave a paper trail for intrepid debuggers to follow.
This method name feels kinda vague. How about
getMostRecentRunTime()or similar?So I really like the method name here, because it makes the intent clear. But "recently" is a vague term and we need to define it more clearly in the doc comment at the very least.
Also, this class is supposed to run multiple readiness checks, right? Depending on their implementation, what if certain ones have been run recently, but others haven't? Is that a potential problem? How do we keep track of this in a unified way? Maybe I'm overthinking it.
Comment #54
phenaproximaA few more thoughts.
Since the results of the readiness checks to eventually become stale, maybe we should use the expirable key-value store (keyvalue.expirable).
This comment is kinda useless :)
I don't hugely love the fact that run() returns an array, which calling code needs to examine to determine if there are any readiness problems. That means the calling code needs to understand the details of how readiness checks work. I'd rather hide those details -- IMHO, something like
$manager->isReadyForUpdates()would be easier to use, and also less prone to being used incorrectly. For example, what I envision:So, to be honest...I'm not 100% certain that there is much benefit to having an interface here. This is a fairly specialized domain object in an experimental module; it doesn't seem like an API we'd want other code to implement. Personally, I feel like the interface is not necessary and should be removed.
Comment #55
tedbow@voleger thanks I haven't had a chance to read that yet but will. Also I open to using a different checker as the first checker to add with this patch but I am not sure of simpler one.
@phenaproxima, thanks for the review!
If we changes this it might be better to refactor at once.
->run()isI think it makes the code more understandable and discoverable. when I am looking at the interface I like to be able to quickly find all the instances of the methods. This allows that. Since it is only on module install I don't think the performance hit makes a difference.
$moduleshere? I thought we did because it is in the hook signature but if don't need$is_syncingin 7) then we don't need$moduleshere for the same reason. I removed it.In this case we don't care which modules where installed we just know the new module might have a readiness checker so we should run the checks.
As for
hook_modules_uninstalled()I guess module that was uninstalled could have checker and the message from the module being uninstalled could be now stored inreadiness_check_results. Obviously we don't want to show a message for module that was already uninstalled so that would be we should all run this inhook_modules_uninstalled().Another option would be to not have both
\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManager::getResults()and\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManager::run()but onlyrun()which returns the results any ways. We could useKeyValueExpirableFactoryinstead ofKeyValueFactorywhich we are using now. This wayrun()could just check ifreadiness_check_resultshad a value or had expired or if$this->getCurrentCheckerIds()didn't match the checkers inreadiness_check_resultsand only then actually run the checkers. Otherwise just return the previous result. Then we won't need\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManager::clearStaleResults()either and probably won't needauto_updates_modules_installed()either. It would just mean that on the status report page if we hadn't run checkers or if the results expired it would actually run them not just get previous results. I am convinced yet they are that expensive to make that a problem.So I am not changing anything for this right now because there will already be a bunch of nits in patch anyways I would rather a refactor like that in its own patch.
I am going to add a new failing test
testReadinessCheckerUninstall()to ensure we solve this problem either by implementinghook_modules_uninstalled()or doing the refactor I suggested.severityis better description but also makes be think we shouldn't be categorizing the services like this.StringTranslationTrait, fixed\Drupal\auto_updates\ReadinessChecker\ReadinessCheckerManager::run()runs all the checkers at once so they all ran the same time. But with my suggestion in 11) maybe won't need this at all. But that would mean anybody who could view the status report could run the requirement checkers(if they hadn't been run recently on cron). But is that problem? The could already see the results and the checker should not be actually making changes, just "checking". If they aren't expensive maybe this isn't a problemre #54
Comment #57
tedbowHere is refactor I suggested in #55. I will wait till is uploaded so I can do a self review by commenting on the interdiff to explain
Assigning to myself because I will do that shortly
Comment #59
tedbowAdded this back because it caused some testing problems. Right now there is no update functionality so I think it confusing to have a "readiness" state for something that is not existing functionality.
Also we only have 1 checker so we can't say it is "Ready" anyways.
we can decide later when want show it in the UI.
I made
run()protected. I think the only drawback of this is that you have to callgetErrors()even when you don't want the results. Here we calling it onhook_install()make sure it is run once.UPDATE: I think maybe we should move this to
hook_modules_installed(). If you are installingauto_updatesalong with other modules that also provider readiness checkers running it inhook_install()may run before the other modules are installed(if they are dependent onauto_updatesbecause they provider unrelated functionality too).Right now you still won't get stale results because
run()makes sure the results were run with the same set of checkers. But it will mean it actually be run 2x when maybe it only has be run 1x.Because
run()checks if checker services have changed this will not actually call the checkers if the modules don't provide new checkers.fixed
Hare to tell from this diff now we don't check when the checkers were last run in
cronbecause we use\Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface::setWithExpire()we don't have worry about it outside of the manager.Removed having checkers only be able return messages in 1 category, errors or warnings. I think this especially makes sense when you realize we don't call this "category" anywhere else in core. These are really severity levels.
When the user is explicitly running the checkers we send the optional
$refreshargument to delete the cache version.Now each checker has to implement
getErrors()andgetWarnings()even if they don't provide both. We could make a ReadinessCheckerBase to implement both and returning empty arrays.I figured the way the FileSystemBase is written if produces and error all checkers that extend it will return the same messages.
I have confirm the contrib module has the same problem
This allows removing
clearStateResults().run()will not never return cached results if checkers have changed.This now makes
testReadinessCheckerUninstall()pass 🎉But it also means that any module removes or adds checker services dynamically stale results won't be returned.
@todo Do we need a test for this?
Because we use
setWithExpire()outside code doesn't have to worry about this\Drupal\Tests\auto_updates\Functional\ReadinessCheckerTest::testCronRun()will now fail until #3113971: Replace REQUEST_TIME in services because you can't fake time when usingsetWithExpire()But since
auto_updates_cron()doesn't have any logic now we probably don't need this test.We probably need a new kernel for
ReadinessCheckerManagerto ensure that works expected if the key/value is manually deleted with a follow to add the cron test back when #3113971 is fixed.Comment #61
tedbowReadinessCheckerManagerComment #62
phenaproximaAlthough the module is hidden, I think we might want to change the description to accurately describe what it does, rather than just say "this module doesn't do any updating". How about: "Experimental module to develop automatic updates. Currently runs automated checks to ensure your site can be automatically updated in the future, but not does not actually perform updates"?
I may be missing something here...but I'm still not super clear on why we need
ReadinessCheckerManager::isEnabled(). Why would anyone ever want to disable the readiness checks, but not the auto_updates module itself? Even if we did allow people to do that, we'd be putting them in a potentially dangerous position (run updates -- which are major surgery -- but don't verify your condition at all beforehand). IMHO, we should lose this method; if people want to disable readiness checks, they should just uninstall the module entirely.@tedbow tells me that this might be something the contrib version of this module has. I was not involved in writing that module, so maybe someone who was, can explain the rationale here?
A relatively minor stylistic complaint here, so feel free to ignore it, but: for better readability, and less repetition of keys, maybe we could structure this function like:
s/manually/now, to match the other place we changed this :)
What if getErrors() and getWarnings() returned render arrays, rather than simple arrays of strings? I can imagine some scenarios where a checker might want to return more detailed information than just a string. Discussion with @tedbow confirms that this is a definite possibility, but it introduces UX issues that @DyanneNova is looking into. So IMHO this needs some sort of plan.
If we remove
ReadinessCheckerManager::isEnabled(), do we still need this link (and the associated settings form code)?It doesn't seem like we need to pass the route object to isAdminRoute(); its documentation says it will fall back to the current route if none is passed.
I kinda wish this were configurable in some way (maybe a flag on the route object, which we could add to these routes via a route subscriber). Not a big deal for the time being, though.
Not sure the word "manually" is what we want here. Maybe we should just remove it outright?
Also, the bigger issue here IMHO is it's confusing that we are linking to the settings form here. Why would we do that? Don't we want to just provide a link to run the checks directly?
Not sure how I feel about this verbiage. We're saying "you can't auto-update until you take further action", but there's no link or anything, and we can't guarantee that the readiness checkers will provide anything actionable here. Maybe we should just strike the second sentence, or change it to a passive voice, like "The following issues must be resolved before your site can be automatically updated"?
"The eligibility for automatic updates" feels like detached, kinda bureaucratic phrasing :) How about something like, simply: "These may affect your site's ability to apply automatic updates"?
This doesn't appear to be used?
Why do these routes need to explicitly specify the _admin_route option? Won't the router automatically figure that out from the
/adminprefix?Supernit: there needs to be a space after
readiness_checkerin the tag name.Comment #63
heddn62.2: We intentionally added it to the contrib module so someone could have it installed and a future core module installed at the same time and not get duplicate messaging. I imagined a day someday, when we'd have the PSA notification but someone wouldn't want to have auto updates enabled. With how we are splitting things up so we are putting PSA into update.module and the auto updates into its own separate module, that need (as is pointed out) is less of an issue. So removing that config option is probably acceptable.
Comment #64
phenaproximaSupernit: "Readiness Checkers" does not need title-case capitalization.
I'm confused. Why aren't we handling the case when errors (or warnings) DO come back from the readiness checkers?
I think the coding standards demand a comment here. Maybe just something a little more descriptive than "Megabyte divisior" :)
I'm very confused by this block; I don't understand what it's doing or why. Can we refactor this now to clarify?
Is this the only place we use static::MEGABYTE_DIVISOR? If so, we should hard-code it. I don't think the mathematical definition of a megabyte needs to be overridden by subclasses, or should be :)
Micro-optimization: maybe we should call getRootPath() and getVendorPath() once at the top of the method, both for readability and so we don't need to keep calling them.
Maybe I'm missing something, but shouldn't $minimum_megabytes be divided by 2 in both of these cases?
According to my research, disk_free_space() can return FALSE on failure, which would break this if statement (and all the other ones too). We should probably have a wrapper method to call disk_free_space() and throw an exception if it returns FALSE.
Might be useful to expand the doc comment a bit to explain why code would want to extend this class, and why it's useful.
I don't think this doc comment is super useful. Maybe "Gets the absolute path at which Drupal is installed"?
Correct me if I'm wrong, but won't this method not work for Composer-native installations that keep
vendornext todocroot? If so, we really need to document that, here and in other visible places.IMHO this should be RuntimeException.
I'm not a huge fan of the short descriptions for these methods, but I can't think of better ones at this moment.
IMHO, this should itself implement
ReadinessCheckerInterface. It is a meta-checker.This is a non-overridable constant, so it doesn't really "default" to anything. I wonder if this is worth making configurable (although IMHO we don't need to expose a UI for it).
Isn't this actually KeyValueStoreExpirableInterface?
$priority can be type hinted as an int.
We need to mention that it defaults to FALSE, and that if it is FALSE, cached results may be returned.
If this can return an int or null, the return type hint needs to be
?int.Supernit: "Ids" should be "IDs".
Comment #65
phenaproximaSupernit: need a space after
priority: 2.The update_test module already has something like this, so I kinda wish there were a generic time_test module somewhere which had this. Not in scope for this issue, though. Maybe a follow-up is in order?
Do we really need to have these constants? It would probably be cleaner to just expose a public static method on this class which sets the fake time, thus keeping all the implementation details internal.
Is there a reason this needs to be static? Also, it's a bit hard to read, can it maybe be reformatted a little?
s/how/who
Not a big deal, but the
testingprofile doesn't enable automated_cron, so this line isn't really doing anything. But it makes sense to have this there as future-proofing in case automated_cron does get added in the future.Maybe we should also assert the absence of the link?
Why is this split across lines?
Oh, man. Embedding emoji directly in tests? You shouldn't give me such ideas...
Er...redirected where, exactly?
The second assertion is basically redundant here.
Yes, IMHO. Both of these things could help the user clear up the errors.
😂 ERR: System currently running on duct tape and prayer.
I think that we should expose a method on ReadinessCheckerManager which explicitly clears the cached results, and call it here.
I feel like also asserting the absence of the bogus message would be good here, no? I guess it depends on what assertReadinessReportMatches() does, which I haven't yet seen at this point in my review.
That's what you think. 😎
This is probably fine as it is, but it will target any details element that contains the words "Update readiness checks". Usually I try to find the summary with those words, then call $summary->getParent()->getText(), for a bit more safety in this regard.
Comment #66
tedbow#62.2
This patch removes the setting enable/disable readiness checkers
Comment #68
tedbowTest failures in #66
@phenaproxima thanks for the reviews!
re #62
update_page_top(), we have a hard-coded list there.Looks like this has been this way all of Drupal 8 and I am guessing longer. I think if we haven't had need for the list of disabled routes for available updates messages to be easily configurable then we won't need it for this.
I don't think we should add any complexity until some has a real world not hypothetical use case for it.
But I don't think it serves much purpose now since we can run the checkers from the status page.
So adding ability to run the checkers directly here.
re #64
1) as of this patch on any admin page that displays the messages that the checker has not be run recently
2) on the status report page
In both case if there are any messages they will be displayed on the page anyways.
I also updated this to check for warnings or errors
The problem is that if you have more than one class extending FileSystemBase then in the case the errors come from the parent they would all have exactly the same message. So the use would see it duplicated.
I refactored this here. Hopefully the new message to the user makes it more clear.
Stopping now but will do 65 next
Comment #71
volegerAddressed "Support finding the 'vendor' directory dynamically". Also added optional argument of vendor dir path for FileSystemBase class constructor.
And fixed typo in the method name -
hasValidVendorPatch->hasValidVendorPathComment #72
benjifisherUsability review
We discussed this issue at #3188068: Drupal Usability Meeting 2020-12-18, so I am removing the tag for a usability review. I have a feeling that it will be added back as this issue gets closer to being done.
I apologize for not reading the earlier comments on this issue. I may be repeating things that have been said before.
Some requirements:
Suggestions:
<details>element (initially closed) for each checker that reports an error or warning. If there are both errors and warnings, and you follow the previous suggestion, then the labels on these Details elements will match the checkers listed at the end of the error message, which will help orient the site administrator.Thinking some more about the last point, my personal opinion is that this is not a good place to use progressive disclosure. Consider three radio buttons: info only, automatic security updates, automatic all updates.
Comment #73
benjifisherWe discussed one more thing at today's Usability meeting: what message to show when Drupal cannot connect to the PSA feed from drupal.org. It is difficult to give actionable advice about how to configure a server that cannot make outgoing requests, but it is easy to give a link to Security advisories > Public service announcements. (Look, I just did it!)
Comment #74
tedbowI didn't think about this at the meeting but currently this is not possible without #309040: Add hook_requirements_alter() because this is separate module without altering the update module. But because this is experimental module I don't think we should do this.
I will put a todo in for #309040.
We had created #3166435: Support alternate 'vendor' folder locations in auto_updates module as a follow-up for this but probably we can just address it here. I think your solution is good but if others think we should do it another way or if it is more complicated than it seems then I think we should push it back to the other issue as follow-up.
Comment #75
voleger#74.3 sure, I agree. Just for clarifying: currently applied detection of the folder based on the presence of the composer Loader class which is always present in the vendor directory. That will introduce accurate vendor directory detection even if the location and the name of the vendor directory were changed through configuration from the composer.json file. That's mean the vendor folder location detection will always respect the current project composer configuration (See an example at the follow-up issue comment). Also, test coverage forced me to introduce an optional manual set of composer directory in the FileSystemBase constructor. From the current perspective not sure when and where it would be useful but at least we have an option to define the location of the vendor dir manually.
Comment #76
phenaproximaI think we should reword this to explain what the module actually does -- not what it will one day do -- but it can wait for now.
Looks like there's an errant blank line here?
The auto_updates.update_readiness route needs the "administer software updates" permission, but we're showing this message to people with the "administer site configuration" permission. If they click the "Run readiness checks" link, won't they get a 403 if they don't have the "administer software updates" permission?
I could be missing something, of course, but if I'm not, then maybe we should have test coverage of this.
This could use a comment to explain why we're getting errors, but not doing anything with them.
Either the word "have", or the word "provide", shouldn't be there.
Maybe we should namespace the tag, like auto_updates.readiness_checker?
Should say "If there are messages, the status page will display them."
But I'm wondering...the status page will also display an "okay" message if everything is hunky-dory, right? So why wouldn't we just call getErrors() here, and immediately redirect to the status report?
If we're keeping this, it should have a period at the end of the sentence.
This method's name is misleading. The "get" suggests that it will return something, but it doesn't. We should probably rename this and make the doc comment more detailed.
Why do we need this? Aren't we kind of duplicating this logic immediately below?
Should probably have a
boolreturn type?This should be "path", not "patch". Also, should it have a
Should be \RuntimeException.
Why are we deleting the cached results in this case?
Nit: Missing a space?
Nit: s/Ids/IDs
Why no return type hints?
Comment #77
tedbowCurrently I just got the only test passing using the changes to
ReadinessCheckerManagerandReadinessCheckerInterface. Since we are now caching not just string messages but summaries too I have createReadinessCheckerResultwhich is value object for storing results from checkersLeaving assigned to myself as I am currently working on this
Comment #78
tedbowre #75
from @voleger
I think we should do what we can not to have this option argument we don't really need.
I have a new
TestDiskSpaceInvalidVendorclass that has an invalid vendor directory like we already have to. Will push up this changeComment #79
tedbow@phenaproxima if you agree will this change I can make and add unit tests.
Comment #81
tedbowI removed the DiskSpace checker from this merge request. I think it should be added in it's own issue. I created #3214654: [PP-1] Create diskspace readiness checker as a follow up.
We have the test readiness checkers that prove the api works.
Comment #83
tedbowSorry I did not update this issue sooner.
Since we worked on this issue it was decided for various reasons explained #3220205: Proposal: As pathway to Drupal core use new 2.x.x branch for Composer Based Updates that we would use the 8.x-2.x branch in contrib.
We have since done and created a issue to opened this #3253158: Add Alpha level Experimental Update Manager module
Marking this a duplicate of that issue