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.
- 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)
- For a project where project datestamp is less than the date on a security release then the security release should be shown
- 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
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3254615-nr-bot.txt | 1.68 KB | needs-review-queue-bot |
Issue fork drupal-3254615
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:
- 3254615-test-dev-security
changes, plain diff MR !1637
- 3254615-remove-still-passes
changes, plain diff MR !1548
Comments
Comment #3
tedbowComment #4
tedbowComment #5
tedbowComment #6
kunal.sachdev commentedI 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.
Comment #7
tedbowComment #8
tedbowComment #10
kunal.sachdev commentedComment #11
tedbowNeeds work for Merge Request comments/suggestions
Comment #12
kunal.sachdev commentedComment #13
tedbowLooks good just needs to have 9.4.x merge and also 1 comment in MR about a @see
Comment #14
kunal.sachdev commentedComment #17
needs-review-queue-bot commentedThe 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.
Comment #20
quietone commentedThe MR is now on 11.x and tests are passing. Time for some feedback!
Comment #22
smustgrave commentedMade 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.
Comment #23
quietone commentedComment #26
catchCommitted/pushed to 11.x and cherry-picked to 10.3.x, thanks!