benjifisher → credited mcdruid → .
The only way seckit can do anything for static assets is if Drupal's private files (or a similar stream-wrapper) is used to serve those files through Drupal / PHP.
Otherwise Drupal doesn't get involved in serving the files at all - the webserver does it without "spinning up" a Drupal / PHP process.
In the majority of cases it's not worth the extra overhead of having Drupal/PHP serve your static files; the webserver is typically really good at doing that job efficiently.
I will close this as "works as designed" as there's nothing for the project to do; this is really down to the configuration of the application and the webserver.
Drupal apps will typically try to let a webserver (e.g. apache, nginx) handle requests for static files such as images, css files etc..
You can make PHP/Drupal handle those requests (e.g. with the private files functionality) but that's usually far less efficient than letting a webserver do the job it's designed for.
That means that Drupal often isn't involved in serving those static assets at all.
If you want to emit particular security headers for them, you'll often have to configure your webserver to do that - e.g. with .htaccess rules or whatever the equivalent is for your webserver of choice.
Some security headers are less relevant when serving static files anyway.
So I think this is "works as designed" unless I'm missing something?
benjifisher → credited mcdruid → .
Just to be clear, by "not yet verified" I don't (necessarily) mean git verified; I was thinking more about this "NEW" status:
I'm not sure how that's reflected in d.o's schema but looks like the proper term is "unconfirmed":
https://www.drupal.org/drupalorg/docs/user-accounts/become-a-confirmed-user →
Adding a user with that status as a maintainer seems fairly bonkers, to use a technical term.
If it's technically possible, we should disallow adding users that are not yet verified as a maintainer of a project.
@ram4nd apologies for missing your comment.
If you can demonstrate an exploitable vulnerability that arises from e.g. having the old copy of jQuery in D7 core when it's not actually used by the application (which uses up-to-date jQuery libraries via jQuery Update), could you please report that in private to the Drupal Security Team? Thanks!
📌 [DRAFT] [META] Drupal 7 policy on jQuery related vulnerabilities Active is also relevant.
Okay to get all "existential" about it (what's the module actually for?)..
The reasons for stripping out all but the final 1.x and 2.x jQuery releases from the module and moving to the "custom paths" approach were explained in detail here:
- #3166985: [Proposal] provide supported / recommended jQuery versions for Security coverage →
- 🌱 Plan for jQuery Update 7.x-4.0 release Fixed
From fairly early on the module provided some CDN integration, and the previous maintainer had argued for a long time that it should focus only on providing integration with CDNs so that jQuery libs could be brought in that way - see #1869928: Better CDN/API/automation support → .
The problem was that nobody actually implemented that improved CDN functionality.
Given the time (and/or community contributions) I'd have happily committed changes to the module that allowed a slick CDN integration where e.g. the admin UI presented a choice of versions available from a choice of CDNs.
However, what we had to settle for instead was a basic implementation which is somewhat DIY for site maintainers - i.e. they have to provide the URLs of a CDN version, or to a local copy.
This certainly isn't fancy (although we did manage to add some functionality to advise site owners when new releases are available etc..) but it solved some of the fundamental problems that the module had whereby it was - at one point - a collection of outdated and insecure copies of the various jQuery libraries, which was for the most part unmaintained but nevertheless used by a large percentage of all D7 sites.
Going back down the path of committing copies of the libraries to the module seems like a big step backwards to me (for the reasons outlined), and I'd need to be convinced that there's a very good reason for doing so.
In the meantime, I'd welcome an MR that provides compatibility shims etc.. for more recent release of jQuery 3.x and other libraries.
There's also:
- ✨ jQuery Cookie 1.4.1 replace with Js-Cookie 3.0.5 and Shim Needs review - seems worthwhile, but has been waiting for (basic) tests for a long time.
- ✨ jQuery 4.0 Support Active
Sorry I think missed an important point in my previous comments.
It's not just that the module gets bloated and messy if it includes a full copy of every upstream release.
It also means that the module maintainers are expected to do a new module release for every (point) release that comes from upstream.
That's probably the most compelling reason to decouple the module from the upstream releases.
The module should handle the integration and any shims etc.. but when it comes to updating to a new upstream release sites should be able to do that for themselves.
(Another reason it makes some sense for the old 1.x and 2.x releases to be included in the module as we don't expect any new releases on those branches).
I'm going to set this to NW as I'm pretty convinced that we shouldn't add the actual JS libraries to the module.
I am of course happy to discuss it further though.
Personally I'd prefer to add the style tweaks / any other shims to the module, plus make any logic changes to e.g. add UI whenever jQuery is being added - if that is necessary..
..but leave the actual libraries out of the module.
The final releases of the 1.x and 2.x branches were retained for legacy / BC, but you can see all the old cruft that was removed in #3311834: remove unsupported JS libraries → .
I would really prefer not to add any more copies of the actual libraries to the module.
They will go out of date and the module will bloat (again) if a new copy has to be committed with every release.
Perhaps after EoL the logic changes, but one motivation for removing almost all of the copies of the jQuery libs from the module was that otherwise there's some expectation that if a copy is included in the module, it's somehow "Supported". Moving to the actual JS code being managed externally removes that; once a release is no longer supported by upstream it's not supported by this module either (sort of with the exception of those two final releases from the 1.x and 2.x branches).
Sorry I haven't had time to review the MR in detail.
Can you please explain why it doesn't work to use custom paths to update to the newest versions of:
* jQuery
* jQuery UI
* jQuery Migrate
?
One of the problems with committing all the JS for specific versions - e.g. jQuery 3.7.1 - is that it's likely to go out of date so quickly.
Do we then add a full copy of 3.7.2 when it's released (leaving 3.7.1 in place too)?
That's how we ended up with tens of megabytes of JS in the module before; most of it outdated.
Sorry, I haven't had time to look at the details properly, but do you need to apply a patch?
Can't you fix your problem with the two variables that control the line-endings?
modules/system/system.mail.inc:55: $line_endings = variable_get('mail_line_endings', MAIL_LINE_ENDINGS);
modules/system/system.mail.inc:65: $headers_line_endings = variable_get('mail_headers_line_endings', "\n");
longwave → credited mcdruid → .
Converted drupal-2783153-45.patch to an MR.
It does seem to cause one test to fail (not verified causation but seems like a strange coincidence if not):
JavaScript 160 passes, 1 fail, 1 exception, and 10 debug messages
---------------------
---- JavaScriptTestCase ----
Status Group Filename Line Function
--------------------------------------------------------------------------------
Exception Warning locale.inc 1527 _locale_parse_js_file()
file_get_contents(misc/drupal-settings-loader.js): Failed to open stream: No
such file or directory
Fail Other common.test 1693 JavaScriptTestCase->testJavaScriptA
When "javascript_always_use_jquery" is FALSE: The front page of the site
does not include Drupal settings loader.
We looked at getting this one into 7.103 but didn't manage to do so.
I think this only makes sense if it makes absolutely no changes on a D7 site that does not enable the option.
The complete lack of JS testing in D7 makes it particularly risky to introduce something like this.
For example @poker10 spotted this straight away:
'misc/drupal.js' => array(
'data' => 'misc/drupal.js',
'type' => 'file',
'scope' => 'header',
'group' => JS_LIBRARY,
'every_page' => TRUE,
- 'weight' => -1,
+ 'weight' => -2,
'requires_jquery' => TRUE,
That would affect all sites whether they implemented the new option or not.
Perhaps that's no big deal, but with no automated testing it's hard to tell.. plus who knows what the consequences would be for real sites with lots of modules installed. Some may have carefully tweaked all of the relative weights.. possibly years ago.
My acceptance criteria for this would be that if the new option is not enabled, absolutely nothing changes.
mcdruid → made their first commit to this issue’s fork.
Sorry didn't mean to change status.
Thanks @greggles
For the Gadget Chains I'm not sure that CWE-502 is a perfect fit, although it's certainly very relevant.
Seems to me that one is tied to the initial vector whereby an application passes untrusted data to unserialize()
, which is not what was addressed in these SAs.
https://cwe.mitre.org/data/definitions/915.html might be a better fit as that's more about the idea that it's possible for an attacker to modify properties of objects in an unintended way such that they can influence the state of the application (with malicious intent).
It seems like these two CWEs are both closely associated with PHP Object Injection, but I think 915 maps better to the Gadget Chains as they are not directly exploitable, but rather represent tools that an attacker can leverage if they find a way to pass their malicious payload to unserlialize()
.
I think CAPEC-586 looks good.
@paul_constantine so to be clear the problem you're having is the message on the admin status report?
This really should have been a warning rather than an error, and I missed that in review:
// See if trusted hostnames have been configured, and warn the user if they
// are not set.
if ($phase == 'runtime') {
$trusted_host_patterns = variable_get('trusted_host_patterns', array());
if (empty($trusted_host_patterns)) {
$requirements['trusted_host_patterns'] = array(
'title' => $t('Trusted Host Settings'),
'value' => $t('Not enabled'),
'description' => $t('The trusted_host_patterns setting is not configured in settings.php. This can lead to security vulnerabilities. It is <strong>highly recommended</strong> that you configure this. See <a href="@url">Protecting against HTTP HOST Header attacks</a> for more information.', array('@url' => 'https://www.drupal.org/node/1992030')),
'severity' => REQUIREMENT_ERROR,
);
}
else {
$requirements['trusted_host_patterns'] = array(
'title' => $t('Trusted Host Settings'),
'value' => $t('Enabled'),
...snip...
One workaround for this which comes to mind is to set a very permissive trusted host pattern such as:
$conf['trusted_host_patterns'] = array('.*');
So that should match anything.
It'd be better to set it up properly if possible.
I'm curious as to how the status report message would not go away if you did set a value for trusted_host_patterns in settings.php - and also how it would not break your site(s) quite badly if you set an example value which doesn't actually match the host / domain in use.
Could you please double check that?
In the meantime, we'll have to decide whether this warrants a follow up / hotfix release or something similar.
Apologies for the disruption; as mentioned I think a warning would have been more appropriate.
https://www.drupal.org/project/drupal/releases/7.103 → was released today.
That's it for the scheduled releases of D7.
Unfortunately we didn't manage to tick off every item on the todo list, but hopefully we're leaving D7 in pretty good shape.
Thank you to everyone that's contributed to D7 over ~14 years and more than a hundred releases.
Thanks everyone!
I think the new patch looks good.
Having yet another conf / variable is not ideal but it obviously avoids the situation where a change like this breaks sites with a certain mail setup and they're left with no choice but to hack core.
@poker10 and I looked at this to do some manual testing; it's a little tricky because MTAs / SMTP servers etc.. tend to be so accommodating of all sorts of weird stuff in email. For example mailpit (very useful tool included in ddev) has a relevant option - which defaults to false:
--smtp-strict-rfc-headers / MP_SMTP_STRICT_RFC_HEADERS
Force Mailpit to return an SMTP error if message headers contain \n instead to \r\n line breaks. By default Mailpit will silently fix incorrect line breaks generated by some broken sendmail clients (see related Github issue).
https://mailpit.axllent.org/docs/configuration/runtime-options/#smtp-server
However turning that on didn't seem to help us verify that anything was broken without the patch. Almost every way of actually testing mail that we tried (including with e.g. mimemail enabled or not) did not seem to show any problem without the patch on PHP8 (although I think we did reproduce html mail appearing a bit broken).
We eventually managed to observe the specific difference that the patch makes using strace to observe what PHP emits when Drupal's mail system sends a message. (FWIW we used https://www.drupal.org/project/drush_debug_tools → which includes a command to send test email from drush - it's old and dusty but still works).
So we verified that without the patch, on PHP 8.2, just a couple of the mail headers were separated by only \n
. For example:
Errors-To: hello@example.com\nSender: hello@example.com\nFrom: hello@example.com
Whereas with the patch applied, this was:
Errors-To: hello@example.com\r\nSender: hello@example.com\r\nFrom: hello@example.com
It looks like - at least in the testing we were doing - those were the only headers being inserted by the code in question. All other headers were not influenced by these changes (and always had the correct line endings).
So I believe that verifies that this patch is having the desired effect.
If it has any unintended consequences with a particular mail set up, the new variable should allow for the change to be overridden without hacking core.
Sounds like it might make sense to add another variable; perhaps one we wouldn't add to default.settings.php if it's intended to accommodate sites with edge case mail setups.
Yeah, I think you're right that we've already addressed the problem described in the IS:
Warning: trim() expects parameter 1 to be string, array given in user_pass_validate() (line 69 of ***/modules/user/user.pages.inc).
It doesn't look like that could happen any more; core's checking is_scalar
pretty early on in the form processing.
This was not the case when the (private) issue was originally filed.
I think we can close this as outdated, but thanks everyone that's worked on it.
#29 looks reasonable but I'd be happier committing this if we could verify that the comment above the changes to the constant is true:
* $conf['mail_line_endings'] will override this setting.
*/
In other words, I'd like to ensure that sites can override these changes if they need to (e.g. because they use a quirky MTA or similar).
An earlier comment in this issue suggested that the $conf / variable does not in fact override the constant.
It'd also be good to see tests passing in an MR with the new d.o testing pipeline.
Great, thanks!
Leaving the "Needs change record" tag in place.
Yeah I'm not sure we need the replacement of the newline characters?
Along the lines of @alexpott's comment (which is now outdated for D10/11, IIUC) are there any other form fields we might want to add this to as well?
I'm thinking particularly of those that are typically exposed to anon users like the (rest of the) login form, perhaps search etc..?
I generally agree with the "core doesn't babysit bad code" approach, but this seems like a simple change which aligns the behaviour of the code with how the docs/comments say it behaves (..and avoids noise in logs when PHP is updated).
Thanks everyone!
One small problem - the test data uses short array syntax (D7 uses the old array()
syntax).
If we can fix that, I'm happy to commit this.
I think this looks good, and would be an excellent thing to add to D7 (even this late in the day).
However it has to be an opt-in change.
Just sanity checking that the code only enforces the host pattern checking if the variable is set:
// Check trusted HTTP Host headers to protect against header attacks.
if (PHP_SAPI !== 'cli') {
$host_patterns = variable_get('trusted_host_patterns', array());
if (!empty($host_patterns)) {
if (!drupal_check_trusted_hosts($_SERVER['HTTP_HOST'], $host_patterns)) {
header($_SERVER['SERVER_PROTOCOL'] . ' 400 Bad Request');
...snip...
So if a site doesn't define any patterns, there's no change.
Site's can add patterns (e.g. in settings.php) and they will then start to be enforced.
If that's all true, +1 from me; I'll double check this and then am happy to commit.
Thanks everyone!
n.b. leaving the "Needs change record" tag in place.
Adding credit.
Thanks!
Thanks everyone!
Agree that this looks like the most sensible pragmatic solution.
Also LOL at the dry humour in https://gitlab.gnome.org/GNOME/libxml2/-/issues/474
Thanks!
Great, thank you!
Less code = more good.
Thanks!
Ah okay, sorry I missed that.
How about CWE-835: Loop with Unreachable Exit Condition ('Infinite Loop')?
Not vastly different to 1050, but IIRC an infinite loop was pretty much the issue in 001, and it looks like 835 is "allowed" (if I'm looking at the right field).
SA-CORE-2024-002 was such a weird edge case that I don't know if we'll find perfect categories for it, but CWE-390 and CAPEC-165 both look like a pretty good match.
+1
Thanks for all the work that's gone into this.
I think SA-CORE-2024-001 might map more closely to CWE-400 than CWE-405.
It looks like 405 relates to amplification (e.g. sending a small message to a DNS server which results in it sending a large response).
400 is about exhausting resources such as memory, which I think is more aligned with the core issue in this case.
longwave → credited mcdruid → .
Tests pass with mysql, sqlite, and postgres (including the new one).
Anyone happy to RTBC this on that basis?
Added a new test that verifies user_requirements()
doesn't incorrectly flag a couple of user accounts that both have their mail set to an empty string.
This passes locally for my with mysql.
+1 to make the scope broad so that there's an option to assign CVEs for EoL versions if that makes sense in particular cases.
I think possibly we could make it clear somewhere that this is not an invitation to file security issues for ancient unmaintained branches, but that doesn't necessarily need to be included in docs on the CNA scope.
I've added the tests that the Security Team used when working on SA-CORE-2024-004; we'd typically wait a few weeks before committing tests that accompany a security fix, but there's no need to do so in this case.
We should add to these tests to verify the behaviour of user_requirements()
when there are multiple users with a blank email.
We might want to account for the fact that could mean a couple of different things e.g. a truly NULL field in the db, or an empty string.
Plus we need to check that all 3 of core's db drivers behave themselves.
The "further steps" were to review and commit the patch that was RTBC'd by several commenters :)
However, certainly appreciate what it's like trying to clean up a messy issue queue!
Ideally we'd add tests to prove the fix.
However, looks like tests have not been committed from the private security issue yet. Perhaps we could do that early in this case (tests that accompany security fixes are often not added for a few weeks to avoid disclosing details of potential attacks, but that doesn't really seem to apply here).
This may be as simple as adding:
$query->isNotNull('mail');
Thanks for reporting this.
https://git.drupalcode.org/project/drupal/-/blob/11.0.9/core/modules/use...
$query = \Drupal::database()->select('users_field_data');
$query->addExpression('LOWER(mail)', 'lower_mail');
$query->groupBy('lower_mail');
$query->having('COUNT(uid) > :matches', [':matches' => 1]);
$conflicts = $query->countQuery()->execute()->fetchField();
if ($conflicts > 0) {
..is how user_requirements()
detects problems.
Looks like we should add a condition to exclude rows where mail is empty / null?
Anyone want to spin up an MR?
I've checked the box so you should be able to edit maintainers now @manarth.
Thanks for your help @vladimiraus!
greggles → credited mcdruid → .
I'm afraid this is definitely a Support Request rather than an obvious problem with the Drupal module.
The issue you're having could be quite specific to your environment.
Making sure StreamMaxLength isn't too low would be one of the first things I'd recommend you check, but looks like you've done that already.
If you really want to know what's happening, it might help to look at the clamd logs which might be at /var/log/clamav/clamav.log
or perhaps going into syslog?
If that doesn't help, you can perhaps use tcpdump or similar to capture the network traffic between Drupal and clamd when a scan fails, in order to see if there are any clues there.
It would be good if the module did a better job of "surfacing" errors if that's possible; for example if the daemon responds with an error message.
Not something I can work on right now, but if there's not already an issue for that in the module's queue it'd be good to file one.
Have you made sure the module is in verbose mode to try to capture as much detail as you can about what's going on?
longwave → credited mcdruid → .
larowlan → credited mcdruid → .
Tested this with up-to-date 11.x and 10.3.x
I do see an error message at /core/authorize.php but the full path is not displayed e.g.
The website encountered an unexpected error. Try again later.
PDOException: SQLSTATE[HY000] [1044] Access denied for user 'db'@'%' to database 'some_invalid_db_name' in Drupal\Component\DependencyInjection\PhpArrayContainer->createService() (line 77 of core/lib/Drupal/Component/DependencyInjection/PhpArrayContainer.php).
update.php just shows a simple "The website encountered an unexpected error. Try again later." message.
I do not agree that this needs a CVE.
bigpipe explicitly sets the secure and httponly attributes to false.
What's the justification for that to be changed?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#block_access_t...
The big_pipe_nojs cookie is just a boolean flag, the purpose of which is explained in \Drupal\big_pipe\Render\Placeholder\BigPipeStrategy
The HTTPonly and Secure attributes are intended to protect potentially sensitive information contained in cookies, and to prevent JS/client side code from being able to manipulate sensitive cookies.
None of that applies to this cookie, so it's legitimate for it not to have those attributes set.
Is the only reason to change them to prevent false positives from security scans?
@ram4nd I think you've pushed a couple of unrelated branches to the MR remote? No harm done, but just wanted to make sure you're aware :)
@poker10 thanks for the review, and yes I think the testing looks okay as it is (I've removed the tag).
I don't love the fallback:
else {
// As a fallback, make sure DRUPAL_ROOT's value is not in the path.
$error['%file'] = str_replace(DRUPAL_ROOT, 'DRUPAL_ROOT', $error['%file']);
}
...but my thinking was that if the value of DRUPAL_ROOT appears somewhere other than at the very beginning of a file path for some reason, it'd be pretty confusing to just make that part disappear. I'm not sure when this would ever happen, but let's say for some reason the docroot path is reflected in a longer path to a network mount or something like that..
I think it's probably worth leaving it in, but I've made a little tweak to the conditional to avoid the fallback running at all when DRUPAL_ROOT is not present in the string in the first place.
A wise man once told me to avoid assignments inside if conditions, but I'd argue it's worth doing here to avoid doing unnecessary work for every error.
If you're happy with this little tweak @poker10 (and assuming tests all pass) I think it's ready to commit.
larowlan → credited mcdruid → .
Looks like the test fail that @catch originally flagged up was under PostgreSQL:
SIMPLETEST_DB = pgsql://drupaltestbot:drupaltestbotpw@database/drupaltestbot?module=pgsql
Not sure how helpful this will be, but debugging MySQL and PostgreSQL side by side, this seems to be where it goes wrong for postgres:
\Drupal\Core\TypedData\DataReferenceDefinition::getTargetDefinition successfully finds a targetDefinition in MySQL but that property doesn't seem to be set in Postgres AFAICS.
I'm not sure why.
The stack is:
DataReferenceDefinition.php:49, Drupal\Core\TypedData\DataReferenceDefinition->getTargetDefinition()
EntityReference.php:52, Drupal\Core\Entity\Plugin\DataType\EntityReference->getTargetDefinition()
EntityReference.php:73, Drupal\Core\Entity\Plugin\DataType\EntityReference->getTarget()
DataReferenceBase.php:37, Drupal\Core\TypedData\DataReferenceBase->getValue()
FieldItemBase.php:154, Drupal\Core\Field\FieldItemBase->__get()
FieldItemList.php:116, Drupal\Core\Field\FieldItemList->__get()
Comment.php:356, Drupal\comment\Entity\Comment->getCommentedEntity()
CommentAccessControlHandler.php:36, Drupal\comment\CommentAccessControlHandler->checkAccess()
EntityAccessControlHandler.php:109, Drupal\Core\Entity\EntityAccessControlHandler->access()
ContentEntityBase.php:738, Drupal\Core\Entity\ContentEntityBase->access()
EntityAccessCheck.php:68, Drupal\Core\Entity\EntityAccessCheck->access()
AccessManager.php:160, call_user_func_array:{/var/www/html/core/lib/Drupal/Core/Access/AccessManager.php:160}()
AccessManager.php:160, Drupal\Core\Access\AccessManager->performCheck()
AccessManager.php:136, Drupal\Core\Access\AccessManager->check()
AccessManager.php:93, Drupal\Core\Access\AccessManager->checkNamedRoute()
Url.php:813, Drupal\Core\Url->access()
StringFormatter.php:132, Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter->viewElements()
FormatterBase.php:91, Drupal\Core\Field\FormatterBase->view()
EntityViewDisplay.php:268, Drupal\Core\Entity\Entity\EntityViewDisplay->buildMultiple()
EntityFieldRenderer.php:257, Drupal\views\Entity\Render\EntityFieldRenderer->buildFields()
EntityFieldRenderer.php:143, Drupal\views\Entity\Render\EntityFieldRenderer->render()
EntityField.php:869, Drupal\views\Plugin\views\field\EntityField->getItems()
FieldPluginBase.php:1195, Drupal\views\Plugin\views\field\FieldPluginBase->advancedRender()
views.theme.inc:231, template_preprocess_views_view_field()
ThemeManager.php:261, call_user_func_array:{/var/www/html/core/lib/Drupal/Core/Theme/ThemeManager.php:261}()
ThemeManager.php:261, Drupal\Core\Theme\ThemeManager->render()
Renderer.php:446, Drupal\Core\Render\Renderer->doRender()
Renderer.php:203, Drupal\Core\Render\Renderer->render()
FieldPluginBase.php:1796, Drupal\views\Plugin\views\field\FieldPluginBase->theme()
StylePluginBase.php:769, Drupal\views\Plugin\views\style\StylePluginBase->elementPreRenderRow()
DoTrustedCallbackTrait.php:107, call_user_func_array:{/var/www/html/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php:107}()
DoTrustedCallbackTrait.php:107, Drupal\Core\Render\Renderer->doTrustedCallback()
Renderer.php:825, Drupal\Core\Render\Renderer->doCallback()
Renderer.php:387, Drupal\Core\Render\Renderer->doRender()
Renderer.php:203, Drupal\Core\Render\Renderer->render()
Renderer.php:120, Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure:/var/www/html/core/lib/Drupal/Core/Render/Renderer.php:119-121}()
Renderer.php:593, Drupal\Core\Render\Renderer->executeInRenderContext()
Renderer.php:119, Drupal\Core\Render\Renderer->renderInIsolation()
StylePluginBase.php:711, Drupal\views\Plugin\views\style\StylePluginBase->renderFields()
StylePluginBase.php:574, Drupal\views\Plugin\views\style\StylePluginBase->renderGrouping()
StylePluginBase.php:462, Drupal\views\Plugin\views\style\StylePluginBase->render()
DisplayPluginBase.php:2177, Drupal\views\Plugin\views\display\DisplayPluginBase->render()
ViewExecutable.php:1592, Drupal\views\ViewExecutable->render()
DisplayPluginBase.php:2467, Drupal\views\Plugin\views\display\DisplayPluginBase->preview()
ViewExecutable.php:1721, Drupal\views\ViewExecutable->preview()
CommentUserNameTest.php:158, Drupal\Tests\comment\Kernel\Views\CommentUserNameTest->testUsername()
This one seems to fail consistently on PostgreSQL as far as I can see; I've reproduced the failure manually with postgres:16 (in ddev).
mcdruid@drupal-10:/var/www/html$ git status
Refresh index: 100% (18217/18217), done.
On branch 11.x
Your branch is up to date with 'origin/11.x'.
mcdruid@drupal-10:/var/www/html$ vendor/phpunit/phpunit/phpunit -c core core/modules/comment/tests/src/Kernel/Views/CommentUserNameTest.php
PHPUnit 10.5.35 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml.pgsql
E 1 / 1 (100%)
Time: 00:02.411, Memory: 4.00 MB
There was 1 error:
1) Drupal\Tests\comment\Kernel\Views\CommentUserNameTest::testUsername
Error: Call to a member function access() on null
...snip...
AFAICS doing composer self-update 2.8.0
looks like it should be a good workaround until this is resolved upstream.
...that is assuming composer does revert the change. If not, we may have to make more significant changes.
I think another test is also affected:
---- Drupal\Tests\Composer\Plugin\Scaffold\Functional\ComposerHookTest ----
Status Group Filename Line Function
--------------------------------------------------------------------------------
Exception Other phpunit-1.xml 0 Drupal\Tests\Composer\Plugin\Scaffo
PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
Bergmann and contributors.
Runtime: PHP 8.3.12
Configuration: /builds/security/drupal-security/core/phpunit.xml.dist
E.. 3 / 3
(100%)
Time: 00:03.088, Memory: 8.00 MB
There was 1 error:
1)
Drupal\Tests\Composer\Plugin\Scaffold\Functional\ComposerHookTest::testComposerHooks
RuntimeException: Exit code: 1
Creating a "fixtures/drupal-drupal" project at "./create-project-test"
Installing fixtures/drupal-drupal (1.0.0)
In PathDownloader.php line 51:
Source path "./drupal-drupal" is not found for package
fixtures/drupal-drupal
mcdruid → created an issue.
Cool, thanks - I've posted in #drupal-infrastructure in slack as I wasn't sure if it was coincidence that the composer version was bumped here..
Thanks for filing an issue in the right place.
I think this may have broken some tests?
See e.g. https://git.drupalcode.org/issue/drupal-3479411/-/jobs/2990908
Drupal\BuildTests\Composer\Template\ComposerProjectTemplates 0 passes 17s 1 fails <===
Drupal\BuildTests\Composer\Component\ComponentsTaggedRelease 4 passes 41s
Drupal\BuildTests\Command\GenerateThemeTest 16 passes 88s
Drupal\BuildTests\Composer\Component\ComponentsIsolatedBuild 24 passes 134s
Test run duration: 2 min 13 sec
Detailed test results
---------------------
---- Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest ----
Status Group Filename Line Function
--------------------------------------------------------------------------------
Fail Other phpunit-1.xml 0 Drupal\BuildTests\Composer\Template
PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
Bergmann and contributors.
Runtime: PHP 8.3.12
Configuration: /builds/issue/drupal-3479411/core/phpunit.xml.dist
..FF 4 / 4
(100%)
Time: 00:16.442, Memory: 14.00 MB
There were 2 failures:
1)
Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest::testTemplateCreateProject
with data set "recommended-project" ('drupal/recommended-project',
'composer/Template/RecommendedProject', '/web')
COMMAND:
COMPOSER_HOME=/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer-home
COMPOSER_ROOT_VERSION=11.0 composer create-project --no-ansi
drupal/recommended-project test_project 11.0 -vvv --repository
/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/test_repository/packages.json
OUTPUT:
ERROR: Running 2.8.1 (2024-10-04 11:31:01) with PHP 8.3.12 on Linux /
5.10.225-213.878.amzn2.x86_64
Reading ./composer.json
(/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer.json)
Loading config file
/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer-home/config.json
Loading config file
/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer-home/auth.json
Loading config file ./composer.json
(/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer.json)
...snip...
/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer-home/cache/repo/file----tmp-build-workspace-142b3cf5f42e57cba5237822b73defbcu4lXMx-test-repository-packages.json/packages.json
into cache
Installing drupal/recommended-project (11.0)
In PathDownloader.php line 51:
[RuntimeException]
Source path "composer/Template/RecommendedProject" is not found for
package
drupal/recommended-project
I can reproduce this by e.g. replacing ddev's current composer (2.7.9) with the phar for 2.8.1 then trying to run just that one test manually:
mcdruid@drupal-10:/var/www/html$ vendor/phpunit/phpunit/phpunit -c core core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php
PHPUnit 10.5.29 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml.mysql
..FF 4 / 4 (100%)
Time: 00:08.780, Memory: 14.00 MB
There was 1 PHPUnit test runner deprecation:
1) Your XML configuration validates against a deprecated schema. Migrate your XML configuration using "--migrate-configuration"!
--
There were 2 failures:
1) Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest::testTemplateCreateProject with data set "recommended-project" ('drupal/recommended-project', 'composer/Template/RecommendedProject', '/web')
COMMAND: COMPOSER_HOME=/tmp/build_workspace_558dd1b0a4860999e23c68f3b3390b60dlYMeC/composer-home COMPOSER_ROOT_VERSION=11.0.6 composer create-project --no-ansi drupal/recommended-project test_project 11.0.6 -vvv --repository /tmp/build_workspace_558dd1b0a4860999e23c68f3b3390b60dlYMeC/test_repository/packages.json
OUTPUT:
ERROR: Running 2.8.1 (2024-10-04 11:31:01) with PHP 8.3.10 on Linux / 6.8.0-40-generic
...snip...
Installing drupal/recommended-project (11.0.6)
In PathDownloader.php line 51:
[RuntimeException]
Source path "composer/Template/RecommendedProject" is not found for package drupal/recommended-project
...snip...
Installing drupal/legacy-project (11.0.6)
In PathDownloader.php line 51:
[RuntimeException]
Source path "composer/Template/LegacyProject" is not found for package drupal/legacy-project
(sorry the test output is quite verbose and it's hard to know exactly what might be significant)
It might not be easy to test the exact scenario from the IS, but I'd think that we should be able to add a basic test that verifies this is working as expected when an error is displayed..
Thanks, new branch / MR which adds a helper function to strip DRUPAL_ROOT from errors that will be displayed.
I'm not sure if replacing the actual path with the text DRUPAL_ROOT
is less confusing than just removing it completely, but it could be a bit misleading if e.g. instead of:
/var/www/html/sites/default/settings.php
..we output:
/sites/default/settings.php
I don't think we want to use something like /path/to/drupal
which would probably also confuse some people as it looks real but is not.
Open to suggestions but as it stands this will show an error like:
No such file or directory in include_once() (line 848 of DRUPAL_ROOT/sites/default/settings.php).
I had a bit of time to tidy this up, but I'd still like to see some new tests added to verify that e.g. the checkbox / variable for the shim works as it's supposed to.
So leaving at NW + Needs tests for now.
benjifisher → credited mcdruid → .
There are a few things to address in the comments, plus needs tests.
Great, thanks for suggesting this @mustanggb, and your help getting it done.
Yeah, I hadn't noticed any problems with those booleans.
However, it does seem like we could make it all a bit simpler and reduce the requirement for mental contortion with those ternaries :)
I think we can do:
'migrateMute' => !(bool) variable_get('jquery_update_jquery_migrate_warnings', FALSE),
'migrateTrace' => (bool) variable_get('jquery_update_jquery_migrate_trace', FALSE),
It's good if we're future-proofing this against the conversion of Drupal.settings to JSON, so it was lucky you had the patch in place.
Thanks for the review.
I've tweaked the MR as we're only adding a new .js file when we need to apply the settings for migrate (I had initially thought the file may apply several different settings).
This hopefully means it's less of a significant / global change, and we're able to stick with the very specific weighting etc.. for the file that's being added to $libraries
.
However, in doing this I've had to add to the js file so that it's doing this:
var Drupal = Drupal || { "settings": {}, "behaviors": {}, "locale": {} };
...otherwise I was getting errors about Drupal
not being defined, and then settings/behaviors/locale
not being defined etc..
As far as I can see this is all working okay; I've tested manually with the jQuery Update defaults and checked with the admin and front-end theme with jQuery Migrate enabled.
My testing hasn't gone far beyond just checking for errors in the console and clicking around a bit to try to verify that nothing's obviously broken.
I would appreciate a review and/or some more manual testing before I commit this.
@mustanggb are you able to take a look please?