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

CommentFileSizeAuthor
#19 interdiff_18-19.txt972 bytesheddn
#19 3087501-19.patch10.26 KBheddn
#18 3087501-18.patch10.18 KBheddn
#18 interdiff_16-18.txt1.61 KBheddn
#16 Screenshot_2019-10-21 Automatic updates log Site-Install(1).png33.31 KBheddn
#16 Screenshot_2019-10-21 Automatic updates log Site-Install.png383.12 KBheddn
#16 3087501-16.patch10.19 KBheddn
#16 interdiff_12-16.txt4.12 KBheddn
#12 interdiff_9-12.txt2.36 KBressa
#12 3087501-12.patch9.57 KBressa
#9 interdiff_4-9.txt15.59 KBressa
#9 3087501-9.patch8 KBressa
#4 3087501-4.patch7.87 KBressa
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented

I'd be afraid with 500+ files changed, that would become a very large single entry and be truncated in the logs.

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented

Yeah, 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

  • /web/core/profiles/testing_requirements/testing_requirements.info.yml
  • /web/core/profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.info.yml
  • /web/core/profiles/testing/modules/drupal_system_cross_profile_test/drupal_system_cross_profile_test.info.yml
  • ...

2. September 2019

  • /web/core/modules/jsonapi/tests/modules/jsonapi_test_field_filter_access/jsonapi_test_field_filter_access.info.yml
  • /web/core/modules/jsonapi/tests/modules/jsonapi_test_normalizers_kernel/jsonapi_test_normalizers_kernel.info.yml
  • ...
ressa’s picture

ressa CreditAttribution: ressa at Ardea commented
Status: Active » Needs review
FileSize
3087501-4.patch7.87 KB

Here 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 under admin/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.

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented

Also, 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?

Status: Needs review » Needs work

The last submitted patch, 4: 3087501-4.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented 
--- /dev/null
+++ b/config/install/views.view.automatic_updates_log.yml

This needs to go optional instead of install 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.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented 
+++ b/config/install/views.view.automatic_updates_log.yml
@@ -0,0 +1,293 @@
+      path: admin/modules/update_log

Can we move this with all other reports? i.e. /admin/reports/udpate_log

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented
Status: Needs work » Needs review
FileSize
3087501-9.patch8 KB
interdiff_4-9.txt15.59 KB

Thanks for the feedback, here's a new patch with the view configuration in the optional instead of install folder, which also puts it under the Reports menu. I added an option of choosing how may lines to show.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Since 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.

hestenet’s picture

hestenet
He/Him
Location Portland, OR 🇺🇸
CreditAttribution: hestenet at Drupal Association commented

administer software updates

definitely seems like the correct permission for this sort of thing.

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented
FileSize
3087501-12.patch9.57 KB
interdiff_9-12.txt2.36 KB

I 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:

...
Drupal\Core\Installer\Exception\AlreadyInstalledException: <ul>
<li>To start over, you must empty your existing database and copy <em>default.settings.php</em> over <em>settings.php</em>.</li>
...

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.

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented
Status: Needs work » Needs review

Changing status.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
Status: Needs review » Needs work

I 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.

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented

Sure, 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?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
interdiff_12-16.txt4.12 KB
3087501-16.patch10.19 KB
Screenshot_2019-10-21 Automatic updates log Site-Install.png383.12 KB
Screenshot_2019-10-21 Automatic updates log Site-Install(1).png33.31 KB

I'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?

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented

I 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?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
FileSize
interdiff_16-18.txt1.61 KB
3087501-18.patch10.18 KB

Mini 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.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
FileSize
3087501-19.patch10.26 KB
interdiff_18-19.txt972 bytes

Full pager. This feature depends on dblog and views being enabled; that's why the tests were failing.

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented
Status: Needs review » Reviewed & tested by the community

Thanks for clarifying IS, and why small pager might give better performance. I tried the last patch (#19) and I think it looks fine now.

  • heddn committed 0073f6d on 8.x-1.x 
    Issue #3087501 by heddn, ressa: How much and how to log when updating in...
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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