lint
and audit
jobs are passing.
Tests are running, but one is failing in Firefox. Can someone familiar with the tests look at it?
One of the audit errors was for eslint
. Upgrading that and eslint-plugin-jsdoc
as required introduced a lot of jsdoc
lint warnings. Therefore, I set --max-warnings=-1
temporarily to keep the warnings from causing job failure. Someone will need to review the warnings and tweak the rules or the code.
I upgraded both the playwright and main docker images.
For the main image, I used drupalci/php-8.3-apache:production
because the drupalci
images are used in the gitlab_templates
project.
I reasoned that there would be maintenance benefits with using the "official" images, and there might be a caching benefit in Gitlab. Also, the drupalci
image already has git
.
Another audit error was for rollup-plugin-terser
, which minifies the build artifact a11y.autocomplete.min.js
. After switching to @rollup/plugin-terser
, the resulting a11y.autocomplete.min.js
is slightly different than before. So, it should be checked.
The browserlist DB still needs updating.
Hid the original MR because the new MR builds on it.
kentr → changed the visibility of the branch 3338664-automated-a11y-tests to hidden.
@the_g_bomb
I think it would be a good idea to move the existing Nightwatch tests to either Playwright or PHPUnit as appropriate when they are available. Just keen to get the current tests working as optimally as possible in the meantime.
Agreed. It sounds like phpunit is the first choice based on 📌 Consider dropping Nightwatch in favor of Functional Javascript tests Active .
I've done some work on 📌 Automated A11y tests in PHPUnit Needs work and replicated the current Nightwatch a11y tests, but hit a snag 📌 Automated A11y tests in PHPUnit Needs work .
FYI, there was some concern about using the standard profile when the nightwatch a11y tests were created: #3293469-23: Automated A11y tests in Nightwatch →
Adding to my last comment:
In case it's preferable to commit the axe javascript file into core, AFAICT it is Mozilla Public License 2.0 and is 541K
minified.
I need implementation advice.
The axe core javascript is an npm package.
In a Slack discussion debugging the failed job, @fjgarlin said that core functional javascript tests don't have access to the core/node_modules
directory. So, the file must be provided some other way.
It has to be built and the dist files aren't in the repo, so it can't be retrieved directly from the repo.
I didn't find any other cases like this, so I experimented with installing it locally via Composer into vendor
by defining a repository in composer.json
as follows and then requiring it as a Composer dev dependency. This is working locally.
Is this a viable solution, or are there other preferred solutions?
{
"type": "package",
"package": {
"name": "npmjs/axe-core",
"version": "4.10.2",
"dist": {
"url": "https://registry.npmjs.org/axe-core/-/axe-core-4.10.2.tgz",
"type": "tar",
"shasum": "85228e3e1d8b8532a27659b332e39b7fa0e022df"
}
}
}
Putting back to RTBC because it was before #62.
Updated issue summary to indicate that the MR to be reviewed is !10482.
In light of
#3467492-15: [policy, no patch] Replace Nightwatch with Playwright →
, I'm working on this against 11.x
and planning to add support for options so that we can use it like we use Nightwatch's axeRun()
.
🌱 [META] improve accessibility of toolbar Active covers these.
I suggest closing this issue.
These don't have their own issues yet, correct?
- Too many ARIA group/landmark roles
- Navigation landmark regions without accessible names
- Empty navigation regions
#3 is a quick fix.
From looking at the template, removing the nested navs as suggested for #1 might also fix #2.
The toolbar is wrapped by an ARIA group (not a landmark) and a navigation role, each with their own label. This double wrapper doesn't serve much purpose, it merely increases verbosity.
It's also throwing this Axe error, which resolves when I change the div
to nav
and remove the role="group"
.
All page content should be contained by landmarks
Ensure all page content is contained by landmarks
Element Location: #toolbar-administration
<div id="toolbar-administration" role="group" aria-label="Site administration toolbar" data-drupal-claro-processed-toolbar="" class="toolbar claro-toolbar toolbar-oriented" data-once="toolbar toolbarAntiFlicker">
Definitely a core issue.
I think it would be great if a11y tests ran automatically for most paths requested by a test. Currently, it appears that Axe tests in Nightwatch are only run for explicitly-defined paths in dedicated tests.
Based on limited experience with Nightwatch, setting up test conditions appears to be easier for PHPUnit functional javascript tests than it is for Nightwatch tests.
It also appears that there are currently more functional javascript tests than Nightwatch tests (at least, going by the number of test files turned up with a quick find
): 242 vs 34.
Doing the a11y tests with PHPUnit might help cast the widest net.
There are some issues sneaking into Core that should have been caught by automated testing.
Some of these missed issues may be a result of the limited nightwatch_a11y_testing
profile. A bunch of Axe errors appear when I use the modules from the standard profile.
I've been digging into it a little more before I create an issue.
It looks like the Api Client module → has been created in the meantime (by @brianperry ;-).
To the general discussion here, a stable & agnostic JS client library that includes admin / content management features could also be used as an interface in Nightwatch commands for setting up a test site.
RE #25, I found these test case URLs.
- "/"
- "/admin/content"
- "/admin/structure"
- "/admin/structure/block"
- "/admin/structure/taxonomy/add"
- "/admin/structure/types/add"
- "/node/add/page?destination=/admin/content"
- "/search/node"
- "/user/1/edit" (tested with and without the
navigation
module - "/user/login"
In these files:
I'm wondering if there's an error in the admin tests.
{ name: 'Create Article', path: '/user/1/edit' },
Should perhaps be:
{ name: 'Create Article', path: '/node/add/article?destination=/admin/content' },
I'm seeing almost identical errors without this module, so these problems may be in core. I'm looking into it further and will file core issues if needed.
Does this issue still need backport to D7?
Drupal core version 7 has reached end of life, and is no longer community supported on Drupal.org.
For me on 11.1.0
(without the patch), the timestamp
column is updated on reads at the interval specified by $settings['session_write_interval']
.
Details of the mechanism ( credit to @znerol for this description 🐛 Session fixation for anonymous users - discard invalid session identifiers instead of accepting them Needs work ):
- Drupal core has the session_write_interval setting which defaults to
180
seconds.- That setting is used in core MetadataBag to initialize its parent.
- The Symfony MetadataBag will update its timestamp when the update threshold is reached.
- When that happens, session metadata is changed and
SessionHandler:write()
is called when the request terminates.- That in turn will also update the
timestamp
field.
Note that this also happens for read-only requests because Symfony's MetadataBag::initialize
is also called when existing session data is loaded for the request.
This test verifies that timestamp
is updated on reads.
Regarding logging users out at a shorter interval: for me it works as expected when I set gc_maxlifetime
and cookie_lifetime
, and do a cache rebuild. The existing cookie isn't cleared, though. It's only working for logins that occur after the settings are changed.
So, the suggested change should not be necessary. It also looks like it writes to the DB after every read, which would have a performance penalty.
I added an empty hook_post_update_NAME()
to the automated_cron
module because the new lazy service closure is on that module.
The pipeline is green, but I could only reproduce the error once locally anyway in spite of multiple tries with full wipe / reset.
Updated the issue summary.
There's an MR for review.
A few FunctionalJavascript tests errored but passed on retry.
FWIW, access to PHP files in sites/default/files
is currently blocked by the root .htaccess
.
With this change, the Security Review overview still states that PHP files in the Drupal files directory can be executed.
However, I've confirmed manually that they are not executed: the raw PHP source is served. It looks like a case of ambiguity in the messaging. Here's the details message:
The Drupal files directory is for user-uploaded files and by default provides some protection against a malicious user executing arbitrary PHP code against your site. Read more about the risk of PHP code execution on Drupal.org.
The .htaccess file is writable which poses a risk should a malicious user find a way to execute PHP code they could alter the .htaccess file to allow further PHP code execution.
I've attached a new PHP-FPM config to reproduce this.
NR for the change that fixed the test.
The remaining failing test was:
1) Drupal\Tests\system\Kernel\System\CronQueueTest::testDelayException
Failed asserting that 0 matches expected 1001.
/builds/issue/drupal-3483996/core/modules/system/tests/src/Kernel/System/CronQueueTest.php:137
I made a change to fix it and commented in GitLab explaining the change.
A couple of other tests failed in the subsequent automatic pipeline, but they passed on with manual reruns:
There was 1 failure:
1) Drupal\Tests\path\Functional\PathAliasTest::testPathCache
Cache entry was created.
Failed asserting that a boolean is not empty.
/builds/issue/drupal-3483996/core/modules/path/tests/src/Functional/PathAliasTest.php:84
FAILURES!
Tests: 4, Assertions: 113, Failures: 1.
There was 1 error:
1) Drupal\FunctionalJavascriptTests\Tests\JSWebAssertTest::testJsWebAssert
WebDriver\Exception\StaleElementReference: stale element reference: element
is not attached to the page document
This test passed when rerun:
1)
Drupal\Tests\file\Functional\DownloadTest::testPrivateFileTransferWithoutPageCache
Correctly denied access to a file when file_test sets the header to -1.
Failed asserting that 200 is identical to 403.
/builds/issue/drupal-3483996/core/modules/file/tests/src/Functional/DownloadTest.php:138
/builds/issue/drupal-3483996/core/modules/file/tests/src/Functional/DownloadTest.php:76
phpDocumentor support is similar to #45, also overriding parameter & return declarations. The layout is similar to the VS Code Intelephense version.
This code:
class A {
/**
* Parent method summary.
*
* Parent method long description.
*
* @param bool $input The input.
*
* @return bool
*/
public function method($input = TRUE) {
return $input;
}
}
class B extends A {
/**
* {@inheritDoc}
*
* Child method additonal details.
*
* @param string $input The input.
*
* @return string
*/
public function method($input = 'default') {
return $input;
}
}
Outputs:
Changes made.
Is this module still relevant? I find that the CKEditor5 inline code button provides this functionality.
My understanding is that updateTimestamp()
is only invoked when session data is saved and the data is unmodified. This matches my observation with the traces.
So, I added trace tests for these cases:
- The previously-stored session is read by a request with the corresponding session cookie.
validateId()
is invoked.write()
is not invoked.updateTimestamp()
is not invoked.
- The previously-stored session is modified by a request with the corresponding session cookie.
validateId()
is invoked.write()
is invoked.updateTimestamp()
is not invoked.
- The previously-stored session is re-saved as-is (unmodified) by a request with the corresponding session cookie.
validateId()
is invoked.updateTimestamp()
is invoked.write()
is not invoked.
The last case should also apply to 📌 Use session.lazy-write once Drupal 8 only supports PHP 7 Active .
regarding {@inheritdoc}...let's leave this up to a core committer to decide.
Got it. I thought it was OK because the phpDoc documentation gives examples of providing additional details along with {@inheritdoc}
.
[StackSessionHandlerIntegrationTest] doesn't cover the situation where the new methods are called from within the php session subsystem
I'll look into these tests.
Yes, you are right. A no-op implementation of updateTimestamp() needs to return TRUE.
Ok. I changed updateTimestamp()
to return TRUE
.
Returning FALSE
from updateTimestamp()
causes PHP warnings and errored tests. I believe the error originates from PHP session.c, line 521.
I pushed it so that others can observe, but I believe updateTimestamp()
should return TRUE
to be seen as a successful no-op.
In Watchdog:
Warning: session_write_close(): Failed to write session data with "Drupal\session_test\Session\TestSessionHandlerProxy" handler in Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->Symfony\Component\HttpFoundation\Session\Storage\{closure}() (line 232 of /var/www/html/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php)
In tests:
Exception: Warning: session_write_close(): Failed to write session data with "Drupal\session_test\Session\TestSessionHandlerProxy" handler
Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->Symfony\Component\HttpFoundation\Session\Storage\{closure}()() (Line: 232)
Suggestions applied. There are a few outstanding threads.
There's an MR for review. I commented on the MR in Gitlab.
There's an MR for review.
I kept the .clone()
because the other changes solved the OP issue of focusing the first new content item and it's unknown why the .clone()
was used.
I added tests and fixed some issues with the preexisting test. There are details in the commit messages. I'm happy to revert anything that's problematic.
As a separate issue, the call to .trigger()
looks incorrect to me for the current version of jQuery. As it is, event handlers receive the new content items as individual arguments instead of as a single jQuery collection object. jQuery docs say to pass the data wrapped in an array or a plain object, but that would break BC.
I'll do it.
@anybody
Could someone look up the issue via GitLab blame and see if there was a reason given?
I don't see a reason. Looks like it was added in commit 95af31da for #3068579: Add a jQuery event when new content is loaded → .
Since the second argument of .trigger()
is used to pass data to event handlers, the .clone()
might have been to prevent mutation of the $newRows
object by buggy event handlers.
There's an MR for review.
In addition to applying the patch from #10, I added support for the views pager Heading Level
option instead of a hard-coded h4
because the accessibility checks were reporting an error about the skipped heading level.
I've tested this manually on D11.
Changing to NW until I create MR.
In line with Drupal's ethos of collaboration rather than competition, I think your effort and great ideas would really benefit the users of one (or all) of the existing modules that are already used in the wild.
There are already accessibility issues in their queues. Adding to Views Infinite Scroll would have a broader reach because it reports many more active installations than Views Load More or Views Show More.
Adding to what @scott_euser said, the topic of model collapse has been in the news recently. From WikiPedia: Model collapse is a phenomenon where machine learning models gradually degrade due to errors coming from uncurated training on the outputs of another model, including prior versions of itself.
I'd hypothesize that the majority of Drupal sites are providers of fresh, quality, human-generated content in many fields. Society appears to be hell-bent on using generative AI as a general tool for real work. So I echo @scott_euser's comment about the danger of adversely-affecting AI output.
This MR makes the test CriticalCssHeadTest
pass, and I verified manually.
Also tested manually with external asset URLs for ✨ Add support for downloading critical CSS from an URL Active .
Disregard my last comment about preprocessing CSS.
It appears that the blank output I'm getting is because the only content of the test CSS files are comments, which are getting stripped by the optimizer.
It appears to also fail for local files when CSS preprocessing is enabled.
In that case, the call to file_get_contents()
only occurs if the file is external.
I think it's actually replacing any found file in CriticalCssProvider.php
with whatever comes after that file in the list, even if that is a value of FALSE
due to a failed file_get_contents()
.
Breaking out of the loop when a file is found fixes it:
diff --git a/src/Asset/CriticalCssProvider.php b/src/Asset/CriticalCssProvider.php index 052b31a..92e40ab 100644 --- a/src/Asset/CriticalCssProvider.php +++ b/src/Asset/CriticalCssProvider.php @@ -195,7 +195,10 @@ public function getCriticalCss() { } else { $this->criticalCss = @file_get_contents($this->matchedFilePath); + if ($this->criticalCss) { $this->criticalCss = trim($this->criticalCss); + break; + } } }
Updated the links to the GitLab instructions and settings. The existing URLs were invalid.
@doxigo, I think there was a misunderstanding.
To your point:
I'd say the
/web/themes/custom/
is the default custom path that people get when they setup a new Drupal website and it is good for new comers to know where things are
I disagree with that.
The concern I had is the forward slash at the beginning of the path that makes it an absolute filesystem path. I highlighted the slash with the caret below:
cd /web/themes/custom/SUBTHEME_NAME ^
I'd say that in most cases when people create a Drupal project it will not be at the root of their file system. So cd /web/themes/custom/SUBTHEME_NAME
will fail.
Instead in those cases, what people would want is to go to web/themes/custom/SUBTHEME_NAME
(without a forward slash at the beginning), because that's relative to the directory that they're currently in.
Added information declaring other components as dependencies.
Vardot section needs updating. Still refers to D9.
Updated phpunit.xml
example to correspond to D11 (11.0.1
).
This page has a lot of overlap with Using Composer to Install Drupal and Manage Dependencies → , and that page is clearer WRT a base installation of supported versions of core. Perhaps it would be good to remove the overlap to prevent duplicate information and/or repackage the non-overlapping content on this page (such as the core development info).
Removed details about installing drupal/core-dev
before installing drush. For me, there were no problems installing drush first.
The linked issues that were linked have both been closed ( https://www.drupal.org/project/drupal/issues/3123013 🐛 drupal/core-dev install fails if drush/drush already installed Closed: outdated , https://github.com/drush-ops/drush/issues/4385), and Composer 2 has been out since Oct 2020.
Automated tests are (rightfully) an important part of Drupal core. After working with Laravel where testing "just works" out of the box with artisan test
, I see getting tests running in Drupal as a hurdle to core development.
Just installed joachim-n/drupal-core-development-project
, and there's still some configuration required that looks like it could be automated. Even after copying the phpunit-ddev.xml
file to phpunit.xml
, I get an error with the --list-tests
option.
To make core dev easy it would cover more ground to automate the setup of underlying dev dependencies without locking people into ddev.
Laravel also has commands to create stub test files. Equivalent commands for Drush would be nice.
container-inline.module.css
could also be moved, and then attached somewhere in the container
chain.
Possible duplicate of
📌
Refactor system/base library
Needs work
, as some work for tablesort
has already been done in that issue.
I did not intend to hide the branch for everyone. I thought that was just for my view.
@jennypanighetti
With regard to the sidebar, a you referring to a visible part of the page? This patch by itself wouldn't add anything visible to a node.
It adds the option to configure the mainEntity
property on a schema.org ProfilePage
item, which will add the property to the schema.org JSON in the HTML <head>
.
Cloudflare doesn't exactly do this. What it does is send Early Hints for subsequent requests (emphasis mine):
Cloudflare takes these [Link] headers and caches them at our edge, ready to be served as a 103 Early Hints payload.
When subsequent requests come for that asset, we immediately send the browser the cached Early Hints response while proxying the request to the origin server to generate the full response.
So the first response through Cloudflare doesn't have the Early Hints headers (as the first response would if Early Hints were served directly by Drupal). But it's better than what we have now, and the Cloudflare cache can be warmed if needed.
As of April 2023 only FrankenPHP supports early hints headers, according to the Symfony blog.
A comment on that post points out a PR to add Early Hints to standard PHP.
However, PHP should currently be able to add Link
headers with preload
or preconnect
.
They would be there to be picked up by Cloudflare and other proxies, and may add small gains even without Early Hints.
A benefit of specifying
preload
in the HTTP Header is that the browser doesn't need to parse the document to discover it, which can offer small improvements in some cases.
@nmangold
I don't love the idea of the catching the error in the console log by default.
Just to be sure, I wanted to clarify that the catch
is what prevents fetch errors from breaking valid code blocks on the page. I defer to you on whether or not the error is then written to the console, but IMO the catch
is necessary (the catch
could absorb the error by default if you don't want the error written to the console).
FWIW, the filter currently accepts <pre><code class="language-XXXX">...
.
For example, this will result in a block of highlighted yaml:
<pre>
<code class="language-yaml">
foo: 'bar'
baz:
- a
- b
</code>
</pre>
I meant core version 10.2.7
in my previous comment.
@andres.torres Except for the alias issues outlined here and the separate issues of 🐛 Filter has false positives Needs review , I've been using this module on a lot of languages without a problem (bash, ini, javascript, json, shell, xml, yaml, probably more).
Currently using v1.1.2
of the module on core 10.1.7
, with the patch in
🐛
Filter has false positives
Needs review
. The browser console error in that case is similar to the error described here.
I wonder if your issue is something besides this one, at least partly.
Excuse me if you know this, I can't tell from your comments:
You can get a raw text patch from a merge request by adding .patch
to the merge request URL, and you can use that URL in composer.json
to patch with composer
. Here for MR #5.
@imalabya: I'm getting an undefined variable warning after applying the patch.
Warning: Undefined variable $match in ajax_loader_page_attachments() (line 81 of modules/contrib/ajax_loader/ajax_loader.module).
I don't see current Drupal standards on undefined variables, but I think eliminating them is helpful if for no other reason than to help more critical errors stand out more.
Adding $match = FALSE;
~ line 57 removes the error.
@mvogel:
I am not sure that enabling ajax loader only on specific pages is a real use case
For me, the use case is keeping ajax loader from loading assets on pages that don't need them. With the current ajax loader functionality, I see the assets / libraries loading on all pages, regardless of whether they have any ajax functionality.
There may be a better way than this change to restrict the loader to only the pages that need it, though.
Having to choose XML for HTML out-of-the-box feels like bad UX to me. HTML isn't a subset of XML.
A compromise is only maintaining a small set of aliases for common cases. I'd put HTML in that group.
That group could be limited to the default languages supported by the core Code Block plugin. Once someone starts adding their own languages to the Code Block config for their site, they can specify the correct aliases.
Of the current Code Block defaults, I believe that the only entries that don't have their own highlight.js language files are HTML and C#.
languages: - label: 'Plain text' language: plaintext - label: C language: c - label: 'C#' language: cs // "csharp" in highlight.js - label: C++ language: cpp - label: CSS language: css - label: Diff language: diff - label: HTML language: html // "xml" in highlight.js - label: Java language: java - label: JavaScript language: javascript - label: PHP language: php - label: Python language: python - label: Ruby language: ruby - label: TypeScript language: typescript - label: XML language: xml
Another solution is using (or offering the option to use) the pre-built highlight.js bundle with common languages. My guess is that would solve most cases, but that's just a guess. I don't know how they decide what languages to include in the bundle.
Granted, loading unnecessary assets is also bad UX. It may be the lesser of two evils for some sites.
I was keeping the merge request as a draft b/c I believed that it needs tests.
However, I'll let the maintainers decide that and have changed the merge request to Ready
.
I'm not seeing this on 1.1.2
for <code>
tags, unless they have a language-*
class
.
🐛 Filter has false positives Needs review should eliminate those.
#17 removed the errors for me on D10.2.4, with no apparent problems.