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
From #3087463-5: Leverage update.xml to discover core security updates
Also, great to see that changed files are now getting logged, thanks! However, since there are so many insertions now, would it be possible to list the files in a single log insertion, or would that become too large?
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff_18-19.txt | 972 bytes | heddn |
#19 | 3087501-19.patch | 10.26 KB | heddn |
|
Comments
Comment #2
heddnI'd be afraid with 500+ files changed, that would become a very large single entry and be truncated in the logs.
Comment #3
ressa CreditAttribution: ressa at Ardea commentedYeah, I thought as much ... Perhaps build a view, and list all the log entries (i.e. files updated) of type
automatic_updates
, and present it under a tab called "Update log"? They could get grouped by date, like this:Automatic Updates Log
The following files were updated:
10. October 2019
2. September 2019
Comment #4
ressa CreditAttribution: ressa at Ardea commentedHere is a rough draft, which might be used as a starting point for a page listing log entries (i.e. files updated) of type
automatic_updates
. I added it as a tab underadmin/modules
("Extend"), mostly because I couldn't add it as a tab on the Automatic Updates page.There should probably also be a way to remove the view from active configuration, should the module be uninstalled and reinstalled, but I am not sure how to do that.
Comment #5
ressa CreditAttribution: ressa at Ardea commentedAlso, when db updates are added to this module, we should probably also log and list them somewhere ("The following updates were run."), to prevent them from drowning in the "Files updated" list. Perhaps they can be listed under a separate "Update log" tab?
Comment #7
heddnThis needs to go
optional
instead ofinstall
folder. Views is not enabled in any of the tests and then this config item breaks many of them. Optional will not install it unless views is also enabled.Comment #8
heddnCan we move this with all other reports? i.e. /admin/reports/udpate_log
Comment #9
ressa CreditAttribution: ressa at Ardea commentedThanks for the feedback, here's a new patch with the view configuration in the
optional
instead ofinstall
folder, which also puts it under the Reports menu. I added an option of choosing how may lines to show.Comment #10
heddnSince it is optional, can we add a Functional test that adds views and verify the view is renderable? I'm mainly interested in what permissions should be attached to the view. I think it should be
administer software updates
, but I'm open to other suggestions.Comment #11
hestenetdefinitely seems like the correct permission for this sort of thing.
Comment #12
ressa CreditAttribution: ressa at Ardea commentedI haven't written a test, or worked with testing before, but I managed to get the core tests sort of running and succeeding in Lando with
~/dev/autoupdate/web/core$ ../../vendor/bin/phpunit modules/datetime/tests/src/Unit/Plugin/migrate/field/DateFieldTest.php
But for some reason I get "already installed"-errors running the two existing tests in this module, as well as the new one, with this:
~/dev/autoupdate/web/core$ ../../vendor/bin/phpunit ../modules/contrib/automatic_updates/tests/src/Functional/LogPageTest.php
I get this result:
So I haven't been able to test this phpunit test locally, but perhaps it can be a starting point? Also, the access permission has been set to
administer software updates
as suggested.Comment #13
ressa CreditAttribution: ressa at Ardea commentedChanging status.
Comment #14
heddnI got to thinking here. There is already a view of all of this. Its the default view from drupal found at
admin/reports/dblog
. And we can pass arguments to the view in the URL. Or if views is disabled, you can set$_SESSION
parameters. See #2904546: admin/reports/upgrade redirect doesn't handle view arguments when enabled as an example of what we'd want to add.Comment #15
ressa CreditAttribution: ressa at Ardea commentedSure, do you mean
admin/reports/dblog?type[]=automatic_updates
, which can be achieved by selecting Type and press Filter?I realize that this is possible, I just didn't mention it in #3 since I thought you were aware of this. But the result is one changed file per line, shown truncated, fx like "/app/web/core/profiles/testing_missing_dependencies/…", so you would have to open each one individually.
My purpose of listing them all in one long list is to gather all changes easily, so that the user can get the complete overview of changes. Or perhaps I don't understand you?
Comment #16
heddnI've some screenshots of what things look like to the IS. If an update flows over the boundary between different minutes, then it will look a little odd. Any suggestions on how to resolve that?
Comment #17
ressa CreditAttribution: ressa at Ardea commentedI don't really have any suggestions about the minute boundary thing ... Any reason you switched to Mini pager? I chose Full pager since there might be a lot of entries, in which case it is nice for the users to be able to jump between the pages, and skip however many they want to.
PS. What does IS stand for?
Comment #18
heddnMini page is better performance. Having a full pager means we have to find out how many results, then print out the numbers. Yet another query. But we can use full pager. It isn't that big of an issue. IS = issue summary.
Here's a test that seems to pass green.
Comment #19
heddnFull pager. This feature depends on dblog and views being enabled; that's why the tests were failing.
Comment #20
ressa CreditAttribution: ressa at Ardea commentedThanks for clarifying IS, and why small pager might give better performance. I tried the last patch (#19) and I think it looks fine now.
Comment #22
heddn