Problem/Motivation

working on #3100110: Convert update_calculate_project_update_status() into a class

I realized this logic in update_calculate_project_update_status() is untested:

// If we're running a dev snapshot and have a timestamp, stop
// searching for security updates once we hit an official release
// older than what we've got. Allow 100 seconds of leeway to handle
// differences between the datestamp in the .info.yml file and the
// timestamp of the tarball itself (which are usually off by 1 or 2
// seconds) so that we don't flag that as a new release.
if ($project_data['install_type'] == 'dev') {
if (empty($project_data['datestamp'])) {
// We don't have current timestamp info, so we can't know.
continue;
}
elseif ($release->getDate() && $project_data['datestamp'] + 100 > $release->getDate()) {
// We're newer than this, so we can skip it.
continue;
}
}

If we alway continue if $project_data['install_type'] == 'dev' the test will still pass

$project_data['datestamp'] is set in \Drupal\Core\Utility\ProjectInfo::processInfoList()
it will be the most recent of the values in the core module info files. The problem is that the datestamp value will only be in the info files if Drupal was downloaded via the zip archive not if the project was started via Composer because it is added Drupal packaging. The code above was written before the people started to use composer with Drupal projects. Actually it was written before Composer was released 😱

According to is set in \Drupal\Core\Utility\ProjectInfo::processInfoList() then if no datestamp is in any info file then datestamp will be set to 0. This will mean that if you are on a dev release then no security updates will ever be shown.

If you are using the zip file and are on dev release then security updates may be shown if the security release is greater than(with 100 seconds buffer) the project date.

We should decide if we still want to show security releases when the site is on a dev release and the project was started from a zip file. We don't do this on Composer based projects.

Proposed resolution

if it is set we need a test:
Have a site that is running a dev release.

  1. for a project that has no datestamp, the Composer example, then datestamp will be set to 0 and no security releases should be shown(according to current logic)
  2. For a project where project datestamp is less than the date on a security release then the security release should be shown
  3. For a project where project datestamp is greater than the date on a security release then the security release should NOT be shown

For setting the timestamps see `\Drupal\Tests\update\Functional\UpdateSemverCoreTest::testDatestampMismatch`

for testing 2 and 3 you could use 1 xml fixture with 2 security releases. 1 security release where the release date is greater than datestamp(+100 seconds) and 1 less. only release with the greater date should be shown on the report.

Then for test 1) neither security release should be shown in the case where there is no datestamp set in the test.

Remaining tasks

Decide what the correct behavior is

CommentFileSizeAuthor
#17 3254615-nr-bot.txt1.68 KBneeds-review-queue-bot

Issue fork drupal-3254615

Command icon Show commands

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

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Title: Most logic around project_status from update XML is not tested » Logic for determining which security releases should be consider if site has a dev release is installed is untested
tedbow’s picture

Title: Logic for determining which security releases should be consider if site has a dev release is installed is untested » Logic for determining which security releases should be considered if site has a dev release is installed is untested
Issue summary: View changes
Parent issue: #3254426: Add tests for logic on project_status from update XML »
Related issues: +#3100110: Convert update_calculate_project_update_status() into a class
tedbow’s picture

Issue summary: View changes
kunal.sachdev’s picture

I looked for the setting of the datestamp field but couldn’t find it. I found out in \Drupal\Tests\update\Functional\UpdateSemverCoreTest::testDatestampMismatch that it is manually set.

tedbow’s picture

Issue summary: View changes
tedbow’s picture

Issue summary: View changes

kunal.sachdev’s picture

Status: Active » Needs review
tedbow’s picture

Status: Needs review » Needs work

Needs work for Merge Request comments/suggestions

kunal.sachdev’s picture

Status: Needs work » Needs review
tedbow’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Looks good just needs to have 9.4.x merge and also 1 comment in MR about a @see

kunal.sachdev’s picture

Status: Needs work » Needs review

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.68 KB

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone made their first commit to this issue’s fork.

quietone’s picture

Status: Needs work » Needs review

The MR is now on 11.x and tests are passing. Time for some feedback!

smustgrave changed the visibility of the branch 3254615-remove-still-passes to hidden.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Made super nitpicky change that I applied directly.

But appears rest of feedback has been addressed.

Random but wonder if in gitlab we could have some sort of coverage checker, so we could see what these new tests are hitting. Just a thought.

Looks good though.

quietone’s picture

Title: Logic for determining which security releases should be considered if site has a dev release is installed is untested » Add tests for determining which security releases are considered when a site is on a dev release

  • catch committed a2007953 on 10.3.x
    Issue #3254615 by kunal.sachdev, tedbow, quietone, smustgrave:  Add...

  • catch committed fbf2b7d5 on 11.x
    Issue #3254615 by kunal.sachdev, tedbow, quietone, smustgrave:  Add...
catch’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Status: Fixed » Closed (fixed)

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