Problem/Motivation

This will be a dumping ground for finding from a code review. If anything becomes more then some simple fixes, we can split them out into their own dedicated issues.

  • ModifiedFiles::_construct() is not consistent with the protected properties -- $this->module $this->profile $this->theme etc.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3094445-12.patch7.55 KBheddn
#8 3094445-8.patch21.19 KBheddn
#8 interdiff_7-8.txt1.77 KBheddn
#7 3094445-7.patch20 KBheddn
#7 interdiff_6-7.txt1.27 KBheddn
#6 3094445-6.patch19.35 KBheddn
#6 interdiff_5-6.txt2.28 KBheddn
#5 3094445.patch17.11 KBheddn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

bdragon’s picture

bdragon CreditAttribution: bdragon at Tag1 Consulting for Drupal Association commented

IgnoredPathsTrait::isIgnoredPath needs a return FALSE; at the end.

Usability (POLA) -- in automatic_updates.module, I was expecting "Your site has not recently run an update readiness check." message to link to the admin interface where you can run it, but it doesn't currently.

Something to test -- Is the code handling Add/Add conflicts? If you touch a file that will be created in an update, does the hash checking code break down?

automatic_updates_cron() doesn't have a last feched cooldown and will re-fetch every time cron runs. This will cause excessive load on the update servers, given that some sites have very short cron intervals.

bdragon’s picture

bdragon CreditAttribution: bdragon at Tag1 Consulting for Drupal Association commented

General: As is seemingly always the case, some of the phpdoc annotations for parameters / returns / exceptions are missing or outdated.

I'm unable to test a core update currently because the release-hashes files are missing for 8.7.9 and 8.7.10 at the moment.

bdragon’s picture

bdragon CreditAttribution: bdragon at Tag1 Consulting for Drupal Association commented

OK, core update tested now that the hashes are up.

* In-place updater leaves files in the OS temp folder. This might cause problems on shared hosting or machines with limited space in /tmp.

* Paths in OS temp folder is fixed, which can cause slowdown as they pile up because the algorithm for FILE_EXISTS_RENAME is "try name, try name_1, try name_2" etc so each new object gets more expensive to create.

* The Disk Space check does not check for sufficient free space in the temporary filesystem.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn as a volunteer and at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
FileSize
3094445.patch17.11 KB

OP: fixed
2.1: fixed
Someone else saw #2.2: #3093866: Add a clickable link to the readiness check status message right after the first install, good catch
2.3: Can you provide more details on what you mean? I think I understand but I'd like to be more clear.
2.4: Will work on that next.
3.1: working on fixing as many of these as I can find.
4.1/4.2: will need to investigate. I'm not sure we want to remove backups and other artifacts because they might be needed.
4.3: Good find, will work on that.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn as a volunteer and at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
Status: Active » Needs review
FileSize
interdiff_5-6.txt2.28 KB
3094445-6.patch19.35 KB

2.4: fixed
4.1/4.3: Did some research on tmp folder cleanup. It seems various OSes have different approaches. The big ones do some form of automated
cleanup. For now, I've added additional logic to the disk space checker specifically for tmp folder. That should keep us from doing wrong.
4.2: this must be from an earlier version of the code. That has changed and now we are doing a different approach.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn as a volunteer and at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
FileSize
interdiff_6-7.txt1.27 KB
3094445-7.patch20 KB
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn as a volunteer and at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
FileSize
interdiff_7-8.txt1.77 KB
3094445-8.patch21.19 KB

  • heddn committed 28113ca on 8.x-1.x 
    Issue #3094445 by heddn, bdragon: Code scan findings
heddn’s picture

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

Committed to 8.x. Needs backport to 7.x.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn as a volunteer and at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
Version: 8.x-1.x-dev » 7.x-1.x-dev
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn as a volunteer and at MTech, LLC, Tag1 Consulting for Drupal Association, European Commission and European Union Institutions, Agencies and Bodies commented
Status: Needs work » Needs review
FileSize
3094445-12.patch7.55 KB

  • heddn committed 319f28a on 7.x-1.x 
    Issue #3094445 by heddn, bdragon: Code scan findings
heddn’s picture

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

Status: Fixed » Closed (fixed)

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