Nice - great to see all the green ticks!
Thanks @ankitv18
Looking good thanks.
At a glance, the stylelint failures look like they should be quite easy to fix?
eslint looks a bit more of a mess, sadly.
Okay, thanks for the explanations and fixes.
Here's the CR about the typedConfigManager / ConfigFormBase deprecation:
https://www.drupal.org/node/3404140 →
At some point we should update the constraints to remove D9, but perhaps that should be 2.1.x
Let's get this merged and then we can look at the other test / static checks fixes in 📌 Fix validate pipeline Needs review (thanks for working on that too!)
Thanks everyone that contributed.
Added a gitlab-ci template.
MR26 is failing most if not all of the tests like this:
29) Drupal\Tests\seckit\Functional\SecKitTestCaseTest::testEnabledReferrerPolicy
Behat\Mink\Exception\ElementNotFoundException: Button with id|name|label|value "Save configuration" not found.
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:158
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:78
/var/www/html/modules/contrib/seckit/tests/src/Functional/SecKitTestCaseTest.php:630
-----
To make this easier to review, can we have some comments / references about where things like the "Handling the BC for typedConfigManager." commit comes from?
Is the solution of removing the constructor completely and calling the parent from the create method a novel solution or is it following a pattern documented elsewhere?
Does this ensure that the deprecation is solved for D11 but the module still works / passes tests for D10.2/10.3?
In order to review these commits properly, I'm having to do quite a bit of digging and research.
It'd be a lot easier if you could give me some clues as to what I'm looking at, and why certain changes are being made. Thanks!
The MR was hard to review with the gitlab template + lots of test changes on top.
I've committed the default template directly as a start. Can we please have a separate issue for any other changes?
MR is currently failing tests AFAICS.
longwave → credited mcdruid → .
@itamair I don't want to interfere in your issue queue, but I'd vote for you to get issue credit on this, and other issues where you've done some great work addressing this incident.
Either way, thank you!
Yeah it's a balance, and I don't think there's a clear "right answer" at present.
Putting a static copy of the library in the module mitigates supply chain risk, but on the other hand if/when a vulnerability is found and fixed in the (upstream) library... we then need to find all copies of it everywhere and update them (or backport the fix).
Which problem is worse?
Referencing dependencies with tools like composer (and perhaps npm in the future?) has the advantage that it makes automated auditing easier (dependabot etc..) and potentially sites can update themselves without relying on Drupal projects doing new releases (that's subject to appropriate constraints being in place, and everyone's interpretation of semver agreeing etc.. etc..)
Apologies - I didn't mean to hide the patch in #3 - I was just adding some tags to try to link related issues together.
Sorry, I was just adding some tags and didn't mean to change the status.
Thank you @torotil - it's only a Security issue in that the trustworthiness of the original 3rd party service has been questioned, so it's probably best not to provide that specifically as an example any more. Appreciate your swift action.
Thanks!
I think it'd be pretty safe for me to commit this directly (and I've already discussed it with @poker10) but NR for now.
@arjun@pennywisesolutions this is not the right place to be asking for help with that. Please have a look for existing issues, file a new Support Request, or ask for help in Drupal slack.
I replied here to make sure that your comment didn't confuse anyone into thinking there was an unaddressed vulnerability.
This issue is closed and doesn't need more comments.
For posterity...
It's worth filing a followup for D7 and D11 to update to 1.5.0
D7 has merged the changes: 📌 Sync D7's copy of Archive_Tar with new 1.5.0 release RTBC
Issue for D11: 📌 Archive_Tar release 1.5.0 Active
Oh, and draft CR looks good, thanks.
I'm not aware of anywhere else that core might be affected by this, no.
We have some quasi-unit tests (which couldn't be actual unit tests as they involve using e.g. the temp directory which means Drupal has to be somewhat bootstrapped IIRC) in SystemArchiverTest (added in
#3102159: Add tests for Archive_Tar →
) if we wanted to add anything specific around this change.
I don't think we specifically need to in this case as we're just mirroring the upstream change, and existing tests cover core's actual usage - I think that's e.g. \UpdateTestUploadCase::testUploadModule which I only found after I'd added the unit(ish) tests mentioned above.
Tweaked the default settings comments on commit, as discussed with @poker10 in chat.
Thanks everyone!
Looks good, thanks.
Does any of this still apply to D10/11?
I'm a bit surprised that there's apparently no existing test of Form API's maxlength but indeed.. I don't see one.
This looks like a good change / BC layer, thanks all!
I thought this one seemed familiar when I started reading :) I suppose both @poker10 and I have written parts of the patch (/MR) so perhaps neither of us should commit it.
However, I think this is okay to commit on the basis that the change is to more cleanly handle the situation where Drupal is going to fail to load a valid controller class anyway. We have tests (thank you!) to prove that the code does what we expect, and multiple reports that (earlier versions) of this change solved (/ mitigated) the originally reported problem.
Also, it's sufficiently long ago that I hardly remember writing the code so it's like reviewing someone else's patch anyway :)
Anyone with a site that hits this error still has some debugging to do to, but it's an improvement on hitting unhandled Fatal errors.
Great - agree it's good to prevent warnings when bots etc.. hit this path.
Thanks!
This is really useful functionality; I've used a script / drush command that does something very similar many times to debug cron problems.
I definitely think that it should not be enabled by default on existing sites though, in order to avoid ballooning log volume.
It looks to me like the latest MR disables the functionality by default in the system module:
/**
* Detailed cron logging disabled by default.
*/
define('DRUPAL_CRON_DETAILED_LOGGING', 0);
...but the comment in default.settings.php suggests that the default is the opposite way around:
/**
* Cron logging.
*
* By default drupal_cron_run() will log each execution of hook_cron() together
* with the execution time. Set this variable to FALSE in order to opt out of
* this behaviour.
*/
# $conf['cron_logging_enabled'] = TRUE;
This should be simple to fix if we're agreed that we just need to make the settings.php stanza match (i.e. default to off).
Thanks - it's a bit circuitous but I think better to avoid potentially(/theoretically) referencing non-existent elements.
Thanks!
This looks good.
I thought I'd spotted a tiny problem in the test because the docs for \DrupalTestCase::randomName() say:
/**
* Generates a random string containing letters and numbers.
*
* The string will always start with a letter. The letters may be upper or
* lower case.
If that was literally true, the test may not always be doing exactly what we want it to.
However, the first letter will actually always be lowercase:
$str = chr(mt_rand(97, 122));
...so the problem there is slightly misleading documentation, rather than a bug in the new test here.
We could fix the docs in a follow-up, but it's not a top priority.
Thanks!
Thank you everyone!
This looks pretty good but I'm not sure about the assertions that reference an element on the return value of variable_get() - e.g.:
$this->assertEqual($file->uri, variable_get('file_test_results', array())['download'][0][0]);
Perhaps it's safe to assume that the variable will always be set and that element will be present, but if we do something similar in isolation:
$ drush php
Psy Shell v0.9.12 (PHP 7.4.33 — cli) by Justin Hileman
>>> print_r(variable_get('file_test_results', array())['download'][0][0]);
PHP Notice: Undefined index: download in phar:///usr/local/bin/drush8/vendor/composer/..eval()'d code on line 1
It may make the code more long-winded but can we avoid this?
CVE-2021-32610 applies to "Archive_Tar before 1.4.14" (emphasis added).
There are no vulnerabilities in 1.4.14 that are fixed in 1.5.0
Drupal core was updated to address the CVE in https://www.drupal.org/sa-core-2021-004 →
FWIW I am now a co-maintainer of Archive_Tar, and was the reporter of that (upstream) CVE.
It's worth filing a followup for D7 and D11 to update to 1.5.0 but doing so would not be a security release.
A dev release is up.
I'm not positive that all of the logic is perfect (e.g. placing generated forum content into containers and selecting parent taxonomy terms) but it seems to do a passable job if you just want to throw together an example forum structure.
Patches welcome, but I don't really anticipate doing more work on this.
I removed some trailing whitespace from a couple of lines on commit; I checked I'd not broken the file with a linter.
Thank you!
Less code is more good :)
Thank you!
Thanks everyone!
I don't think we need to jump through hoops to backport the test to D7.
However, per #2927012-33: _drupal_log_error() returns a 0 exit code on errors → it deserves a Change Record as it may cause a change in the behaviour of tests.
benjifisher → credited mcdruid → .
I tweaked the module name in the update hook per #13.
I've also changed it so that standard_install() only switches on the opt-out flag for the specific test class that otherwise fails. This meant adding a new element to $GLOBALS['drupal_test_info'] which feels a bit icky, but I think it's a reasonable compromise.
This means we're getting all tests to pass, plus testing the opt-out itself (within the profile if not the update hook) but all the other tests that use the standard profile will get the module enabled by default. That's good because it means we're more likely to catch any regressions involving the new module.
With the module enabled by default in the standard profile, we could probably tweak some of the tests in announce_feed_test.test where it shouldn't be necessary any more to explicitly enable the new module, but I don't think we need to do that immediately (and it shouldn't do any harm if we never make that change).
Erm, incidentally, should announce_feed_test.test be just announce_feed.test? Again, not an urgent thing to address.
Tests pass with that hack.
Manually testing the opt-out flag in settings.php
without:
$ ddev drush -y si && ddev drush pml | grep -i announcement
...snip...
Installation complete. User name: the-head-honcho User password: ...snip...
Core Announcements (announcements_feed) Module Enabled 7.100-dev
with the opt-out:
$ ddev drush -y si && ddev drush pml | grep -i announcement
...snip...
Installation complete. User name: the-head-honcho User password: ...snip...
Core Announcements (announcements_feed) Module Not installed 7.100-dev
I've also manually tested system_update_7087() with and without the opt-out variable set.
So the only remaining question for me is whether we can think of a cleaner way to achieve something similar to the check for the drupal_test_info global in standard_install()?
We also need to write the CR and Release notes snippet.
Per @poker10 the problem is that ModuleUnitTest::testModuleList() compares the list of modules from .info files with what module_list() reports has actually been enabled.
With any luck, the commit I just pushed should make the test pass, as we set the opt-out flag when the standard profile is being installed inside a test.
As an added bonus, this also tests the opt-out itself (at least in the install profile).
I would like to find a cleaner way of doing this though; I tried adding a \ModuleUnitTest::setUp() in which to set the opt-out variable, but that doesn't work because the parent setUp() method wipes $conf clean before installing the test site.
We could mess about more with setting a global there, and checking for it during the install in \DrupalWebTestCase::setUp but I'm not sure that'd be any better than adding the check in standard_install()?
Thanks!
@Fabianx suggested that the opt-out should also work for a fresh install, which seems reasonable but means we'll need a slightly different approach.
I've removed the entry from standard.info and put the same code that checks the opt-out in the system module's update hook into standard.install.
For a quick manual test, here are the relevant watchdog messages after running a drush site-install:
38 05/Mar 08:25 actions notice Action 'Unpublish comment' added.
37 05/Mar 08:25 actions notice Action 'Publish comment' added.
36 05/Mar 08:25 system info standard module enabled.
35 05/Mar 08:25 system info standard module installed.
34 05/Mar 08:25 system info announcements_feed module enabled.
33 05/Mar 08:25 system info announcements_feed module installed.
32 05/Mar 08:25 system info toolbar module enabled.
31 05/Mar 08:25 system info toolbar module installed.
The module seems to be enabled just before the list of dependencies in the .info file, which should be fine in this case.
I've also done a manual test to confirm that adding this to settings.php
$conf['announcements_feed_enable_by_default_opt_out'] = TRUE;
...means that the module is not enabled during installation.
I think it's preferable to do this in the standard install profile so that we don't install the module as part of the minimal profile, as that doesn't seem like it'd be appropriate.
I think it makes sense to enable announcements_feed by default in the standard profile, but not minimal.
I've pushed a commit for that to the MR.
As this is done via an entry in the .info file of the profile, the opt-out variable won't do anything there... I think that's okay though. It's really for existing sites to opt-out of the module being enabled by the update hook.
@Fabianx approved this in a different channel. However I won't mark it as RTBC just yet...
We need to commit the actual module first, and will then perhaps need to start a new MR.
@poker10 has also pointed out that we need to consider whether this is enabled by default for a clean install. I think it probably should be.
@Fabianx has approved this (and the opt-out) variable in a different channel.
I think that means it's ready to commit.
I think maybe just keep it really simple.
I've pushed a very basic update hook with a comment that looks like this via drush:
$ drush updb
System 7087 Enable the Announcements Feed module - see Release Notes for opt-out details.
I'd suggest that we don't add the opt-out variable to default.settings.php as this is a one-off situation; once the update hook has been run, the variable won't do anything.
So if this looks like it'll do the job, we just need to write up the CR and snippet for the Release Notes.
Thought about adding a link to the (currently draft) CR to the hook_update_N() comment to clearly signpost instructions for the opt-out - e.g.:
/**
* Enable the Announcements Feed module by default - see https://www.drupal.org/node/3424912 to opt out.
*/
function system_update_7087() {
// Allow opt-out.
if (!variable_get('announcements_feed_enable_by_default_opt_out', FALSE)) {
module_enable(array('announcements_feed'), FALSE);
}
}
However this ends up being output like this by drush:
$ drush updb
System 7087 Enable the Announcements Feed module by default - see https:www.drupal.orgnode3424912 to opt out.
..so that doesn't quite work.
I didn't mean to set this back to NR; I've only made small tidy up changes since @poker10 RTBC'ed in #91.
Back to RTBC; I think this is ready for a final Framework Manager review.
haha; wondered why a couple of $variable names didn't show up in those last commit messages.. it was exactly the same as https://www.drupal.org/project/cvslog/issues/563110 → - apparently I need more coffee.
Just noticed that we're creating the $user twice in the newest test.. I probably copy-pasted that from one of the other tests.
Should be an easy thing to tidy up before commit.
@poker10 I've hopefully addressed your latest round of feedback.
I've just pushed a new template_preprocess_announcements_feed() function similar to the one in aggregator.
Also noticed that the template should have a hyphen in the name rather than an underscore.
Tests pass with this change locally.. we should add at least one test that checks the sanitisation actually works though.
I think there's one more item in the feedback to address; no idea why we've ended up with tabs in a file. I'll work out whether to fix it in vim or emacs :)
Good question on security / sanitisation. Of course we'd hope that we can trust content from this source, but we all know what assumptions lead to.
The aggregator module is hopefully a good reference; here's where it sanitises feed items:
https://git.drupalcode.org/project/drupal/blob/7.99/modules/aggregator/a...
It doesn't feel ideal to duplicate aggregator_filter_xss() but we can't rely on that module being enabled.
Do we need an allow-list like that for filtering, or would using defaults work?
I believe I've addressed all of the outstanding feedback in the MR, including my own :)
@mitthukumawat not sure if you're still working on this, but I have a little time today so am assigning it to myself to attend to the most recent feedback.
Thanks for the offer - I've added you as a maintainer.
This new feature makes me slightly nervous, but I can see why it might be a pragmatic solution in some cases.
I'm pleased to see that there's logging, a specific permission for it, and that it's not enabled by default.
It'd be great if you can do some housekeeping on the module, including getting it setup for properly for gitlab.
Thanks again.