🇵🇪Peru @marvil07

This is ready for review.
The 3448936-kc-integrity-check and related MR !260 contains the proposed code.

The 3448936-kc-integrity-check-batch branch is there just for comparison.
Using Batch API helped a bit with the performance of the implementation, but the cost was a lot higher complexity.
I would say that performance gain may not be worth in this case.
Hence, no MR opened related to this branch.

It turns out that this was skipped on the codebase.
These changes remove that skipping.

After that, and changing configuring to add mappings, I verified that both username and its e-mail are changed on login.

New commits on new 3452530-name-email-sync topic branch and related new MR !111.

- 130217f Allow mapping user name and email from IdP

marvil07 → created an issue.

marvil07 → created an issue.

Rather than module weighting, hook_module_implements_alter() is a bit nicer to control hook ordering, in case some hooks need to fire early and others late.

@drumm, that makes sense.

I have added an implementation of that hook moving the new hook_user_logout() to be executed at the end.

New commits on the 2882270-logout-support topic branch and related MR !107:

- 56dfedf Move hook_user_logout implementation to the end of the set

@xaris.tsimpouris, thanks for starting this!
And to everyone in the thread that pushed it forward.

I have been trying the latest iteration at #31, and it seems to work OK 👍

I have iterated over it, and added some changes on top, but first, let me reply a bit to previous comments.

The comment at #16 is quite relevant:

By the way, this also means any hook_user_logout() implementations that are executed after this one get skipped entirely. So it's potentially a bigger problem than the session data simply being unavailable to them. @solideogloria is right, this could require adjusting the module weight.

⚠️ In other words, using this patch also means that the site may need to adjust the module weight for this module, so it runs at the end of the set.
There seems to not be a way around it, since a redirect is quite definitive.

Actually the watchdog line in user_logout_current_user() is still called (I see the message in the dblog), so I don't think we're preventing execution of other code...

Re #17: The watchdog message happens before invoking the hooks, hence it will appear at the start.
E.g. if module A and openid_connect are implementing hook_user_logout(), and the openid_connect module gets called first, the A_user_logout() hook will not be run.

Since September 12, 2022 there is an official spec for this.
See https://openid.net/specs/openid-connect-rpinitiated-1_0.html

Re #30: Yes.
Looking into the way this works, and the spec about Relying Party initiated logout linked, I think these changes actually implement it.

On masquerade module support, I added a check to verify if the module is there on the related code, since it may not be there.

I have also added the client identifier as parameter on the redirect, which is part of the optional parameters to pass on the spec, and will add an extra hint to the IdP that will produce an extra check.

Also, I have been trying to avoid the persistence of the token data into the session, but I could not arrive to a working solution.
I even tried to use the internal mechanism for redirect, but I could not make it work, basically the following hunk.
Ideas are welcome about how that may be possible.
In the mean time, I restricted this to only the token_id, since that is the only consumed piece.

    diff --git a/plugins/openid_connect_client/generic/OpenIDConnectClientGeneric.class.php b/plugins/openid_connect_client/generic/OpenIDConnectClientGeneric.class.php
    index 9db59a5..51fff9e 100644
    --- a/plugins/openid_connect_client/generic/OpenIDConnectClientGeneric.class.php
    +++ b/plugins/openid_connect_client/generic/OpenIDConnectClientGeneric.class.php
    @@ -66,7 +66,12 @@ class OpenIDConnectClientGeneric extends OpenIDConnectClientBase {
           'post_logout_redirect_uri' => htmlentities($post_logout_redirect_url),
           'client_id' => $this->getSetting('client_id'),
         ));
    -    drupal_goto($endpoints['end_session'], $url_options);
    +    $_SESSION['openid_connect_destination'] = array(
    +        $endpoints['end_session'],
    +        $url_options,
    +    );
    +    $_SESSION['openid_connect_op'] = 'logout';
    +    drupal_goto(OPENID_CONNECT_REDIRECT_PATH_BASE . '/' . $this->getName());
       }
     
     }

I opened a 2882270-logout-support topic branch and related new MR !107 with the following commits.

• 73288fd Add patch from #2882270-31 by solideogloria, xaris.tsimpouris, romanos, nrogers: Support to logout from the OAuth2 Server provider
• 17462c5 Formatting related changes
• 8bcd65e Link to relying party initiated logout, not to session management logout
• c8b3e9c Pass client identifier on logout request
• f453cce Check for masquerade module before checking its data
• c9edac6 Only store id_token for endSession
• 1f9b430 Trust openid_connect_get_connected_accounts() output
• 248beb3 Skip watchdog and show message on no IdP logout
• 09e48d8 Tweak logic on hook_user_logout() implementation

marvil07 → made their first commit to this issue’s fork.

drumm → credited marvil07 → .

@justafish, thanks for opening this issue, and providing a clear test change to reproduce it 👍

@wotnak, thanks for implementing a fix! 👍

Apart from the test fix, I also verified with another use-case on a custom theme that the default value is being used.

@smustgrave, I have updated the issue summary with a few more details.

Marking as RTBC.

@drumm, Thank you!
It seems that everything went well 👍

marvil07 → created an issue.

BTW this can be tested over sso-drupal dev-www site, where dev-drupal KC instance is used, right now with differing counts.

I started exploring a bit what KC API can offer.
The user count endpoint is used.

To provide a diff, the user retrieve may be the one to use; since it will contain basic information including user attributes, like sub, that is the uid in Drupal.
But the default is 100 maximum, and it may not be possible to retrieve all the users for a big user set.
Deferring that for now.

So, instead I focused on a simple counting report for this command.

New commits on new 3437760-kc-user-count topic branch and related new MR !243

- 13f58d31 Add new drupalorg-keycloak-users-count drush command

marvil07 → created an issue.

BTW I tested this over sso-drupal devwww instance.

Added a change to use the new right data source for the created time stamp on the integrity verification.

New commits on new 3436607-keycloak-created topic branch and related new MR !237

- 6d212161 Use the new KC createdTimestamp property for integrity check

marvil07 → created an issue.

Opened MR !18 with new 3428917-rm-keycloak-plugin topic branch related to it.

- 3e21f3f Remove custom keycloak openid_connect plugin

marvil07 → created an issue.

Opened related MR !90 about this new feature with 3427939-dom-str-replace-text-mode branch.

- 3e417d1 Add text mode to dom_str_replace migrate process plugin

Marking as NR.

marvil07 → created an issue.

I was also trying to do a replace with empty today, and noticed the problem.

But I can also see the merit of @koosvdkolk's suggestion in #2, because (again, h/t @danflanagan8 on Slack), maybe no other options should be allowed to be empty?

Exactly, in all other cases options needs a value, so allowing empty of them may not make sense.

I ended up with a variation of the same idea in the code above.

I opened related MR !89 about it, with one commit.

- 804f2e2 Allow replacing with an empty string on dom_str_replace migrate process plugin

Marking as NR.

marvil07 → made their first commit to this issue’s fork.

marvil07 → created an issue.

marvil07 → created an issue.

It will be nice to have a way to walk through a set of items nested in all the item selector items.

I have started one custom data parser plugin that does that.
It basically extends the json data parser plugin, and then uses a given selector to unify its values across all the elements.

E.g. the following the original example, the following source configuration bits will extract the four items.

data_parser_plugin: json_subitem
item_selector: Centers
subitem_selector: Sets

On the ideal world, jq pipelines can do the work once, but if the source endpoints cannot be changed, something like this data parser helps to produce a set that is sensible to walk.

New commits on new 3087614-json-subitems topic branch and related new MR !87.

- 3f81967 #3239180: Start a JSON subitems data parser

Suggest how to use an SDC with include twig tag, complementing include function.

I have verified that the changes on the MR solves the errors as reported by config_inspector → .

On 6.1.1 git tag.

$ drush config:inspect --filter-keys=gathercontent.settings,gathercontent.import
➜  🤖 Analyzing…

 Legend for Data: 
  ✅❓  → Correct primitive type, detailed validation impossible.
  ✅✅  → Correct primitive type, passed all validation constraints.
 ------------------------ ---------- ------------- ------ 
  Key                      Status     Validatable   Data  
 ------------------------ ---------- ------------- ------ 
  gathercontent.import     2 errors                       
  gathercontent.settings   2 errors                       
 ------------------------ ---------- ------------- ------ 

On the `3414140-config-schema-fixes` branch on the MR.

$ drush config:inspect --filter-keys=gathercontent.settings,gathercontent.import
➜  🤖 Analyzing…

 Legend for Data: 
  ✅❓  → Correct primitive type, detailed validation impossible.
  ✅✅  → Correct primitive type, passed all validation constraints.
 ------------------------ --------- ------------- ------ 
  Key                      Status    Validatable   Data  
 ------------------------ --------- ------------- ------ 
  gathercontent.import     Correct   60%           ✅❓  
  gathercontent.settings   Correct   50%           ✅❓  
 ------------------------ --------- ------------- ------ 

Marking as RTBC 👍

Added one configuration schema change at MR !7.
I first tried to use a boolean type, but when using configuration export, I see a string, so using that for now.

marvil07 → created an issue.

@heddn, I have added types and labels to the new client_options key.

Please notice that even if API documentation uses types like boolean, or array of type; at the end they are represented as a comma separated values string.
Hence, I am reflecting that instead, given the added option tries to reflect directly what will be passed to the underlying library, and ultimately to the API.

New commits added to 3420528-add-source-filters topic branch and related MR !14

- 53dcd97 Add types and labels to client_options mapping on source schema

I have added a possible implementation on a new merge request.
From the description there:

The underlying used library receives and passes the request query options to the List items request on the API.

In the same spirit, after exposing a way to pass directly those options in the migrate source plugin configuration, filtering is expanded to support any of the request query options in the API.

New commits on 3420528-add-source-filters topic branch and related new MR !14

• 102e629 Allow GC client list items request query options override
• 362f056 Declare new schema for migrate_plus.source.gathercontent_migration configuration
• d76227d Directly use migrate source plugin configuration
• b6de622 Include list items request query parameters on configuration schema

marvil07 → made their first commit to this issue’s fork.

@eelkeblok, thanks for reporting this problem, and also providing a solution for it! 👍

I got into the same problem, and the changes on the MR solved it!

BTW, both phpstan and phpcs reports that are not passing, are the same as the status on current 2.x branch; in other words no new warnings around those checks has been introduced in this patch.

Marking as RTBC.

@andileco,

Thanks for offering help to co-maintain 👍
I would like to work together on some issues before adding you as co-maintainer.

Please find the main next steps for this module over #2843192: Port vote_up_down to D8 → .
I will be happy to try to review any patches to those issues.

Also, please notice that this module depends on voting_api → , which does not yet have a stable D8+ release, #2574687: [votingapi] Voting API → .
Any help there will also help here.

Finally, there is a core issue that needs help too, 🐛 "nojs"/"ajax" route parameter in use-ajax link breaks CSRF protection Needs work , which this module depends upon for D8+ versions.
Help there is also needed, so a release can be created, without depending on a core patch.

hestenet → credited marvil07 → .

hestenet → credited marvil07 → .

@mstrelan, thanks for the extra test fixes!

@smustgrave, thanks for the extra review, I replied to your comments.

Line numbers

The error is related to line numbers.
One of the fixed tests needed a change of line numbers, because the new naturally declare() affects line numbers.

The new MR contains the changes without the declare() additions, since the task has been split in two steps.

In other words, we need to change line numbers in the test check when we are not adding the declare statements.

I have changed the rebased MR to match line numbers as needed.

using t()

As mentioned on the comment in the MR, we could change multiple tests to avoid using t(), but IMHO that is out of scope of this issue.
More details on my MR comment.

Next steps

Now that tests are passing again, and the threads on the MR has been replied to, I moving the issue back to NR.

CI reports some errors at ConfigSchemaTest and ResponsiveImageFieldDisplayTest tests.
Both of them seem to be relevant, so moving to NW.

Several of them may be about actually changing the test, that was assuming a different context, like the png string match.

Copying here the relevant output from each of them in CI output.

---- Drupal\KernelTests\Core\Config\ConfigSchemaTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-175.xml      0 Drupal\KernelTests\Core\Config\Conf
    PHPUnit Test failed to complete; Error: PHPUnit 9.6.15 by Sebastian
Bergmann and contributors.

Testing Drupal\KernelTests\Core\Config\ConfigSchemaTest
..F........                                                       11 / 11
(100%)

Time: 00:12.394, Memory: 4.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\Config\ConfigSchemaTest::testSchemaData
Got an array with effects for image.style.large data
Failed asserting that actual size 2 matches expected size 1.

/builds/issue/drupal-3406267/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3406267/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-3406267/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php:360
/builds/issue/drupal-3406267/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 11, Assertions: 74, Failures: 1.

---- Drupal\Tests\responsive_image\Functional\ResponsiveImageFieldDisplayTest ----


Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Exception Other      phpunit-3.xml        0 Drupal\Tests\responsive_image\Funct
   PHPUnit Test failed to complete; Error: PHPUnit 9.6.15 by Sebastian
Bergmann and contributors.

Testing
Drupal\Tests\responsive_image\Functional\ResponsiveImageFieldDisplayTest
EE....E                                                             7 / 7
(100%)

Time: 01:04.705, Memory: 4.00 MB

There were 3 errors:

1)
Drupal\Tests\responsive_image\Functional\ResponsiveImageFieldDisplayTest::testResponsiveImageFieldFormattersPublic
Behat\Mink\Exception\ExpectationException: The string "type="image/png""
was not found anywhere in the HTML response of the current page.

/builds/issue/drupal-3406267/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-3406267/vendor/behat/mink/src/WebAssert.php:324
/builds/issue/drupal-3406267/core/tests/Drupal/Tests/WebAssert.php:540
/builds/issue/drupal-3406267/core/modules/responsive_image/tests/src/Functional/ResponsiveImageFieldDisplayTest.php:333
/builds/issue/drupal-3406267/core/modules/responsive_image/tests/src/Functional/ResponsiveImageFieldDisplayTest.php:93
/builds/issue/drupal-3406267/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

2)
Drupal\Tests\responsive_image\Functional\ResponsiveImageFieldDisplayTest::testResponsiveImageFieldFormattersPrivate
Behat\Mink\Exception\ExpectationException: The string "type="image/png""
was not found anywhere in the HTML response of the current page.

/builds/issue/drupal-3406267/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-3406267/vendor/behat/mink/src/WebAssert.php:324
/builds/issue/drupal-3406267/core/tests/Drupal/Tests/WebAssert.php:540
/builds/issue/drupal-3406267/core/modules/responsive_image/tests/src/Functional/ResponsiveImageFieldDisplayTest.php:333
/builds/issue/drupal-3406267/core/modules/responsive_image/tests/src/Functional/ResponsiveImageFieldDisplayTest.php:103
/builds/issue/drupal-3406267/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

3)
Drupal\Tests\responsive_image\Functional\ResponsiveImageFieldDisplayTest::testResponsiveImageFieldFormattersMultipleSources
Behat\Mink\Exception\ExpectationException: The pattern /\s+\s+\s+\s+/ was
not found anywhere in the HTML response of the page.

/builds/issue/drupal-3406267/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-3406267/vendor/behat/mink/src/WebAssert.php:354
/builds/issue/drupal-3406267/core/modules/responsive_image/tests/src/Functional/ResponsiveImageFieldDisplayTest.php:509
/builds/issue/drupal-3406267/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

ERRORS!
Tests: 7, Assertions: 187, Errors: 3.

I have changed the approach to use phpcs configuration as the base for modifying the files in bulk.
That produced a few more changes.
I manually modified some of the files to move the declare() after the @file docblock, since it seems like in those cases phpcbf is adding an extra @file section after adding the declare().

I rebased the added changes on top.
And then, I continued with fixing errors as produced after the changes.

At this point four tests are missing to be fixed:

• Drupal\FunctionalJavascriptTests\Core\CsrfTokenRaceTest
• Drupal\Tests\announcements_feed\FunctionalJavascript\AlertsJsonFeedTest
• Drupal\Tests\page_cache\Functional\PageCacheTest
• Drupal\Tests\system\FunctionalJavascript\Form\TriggeringElementTest

Changes now on 3402007-strict-types-test-modules-testonly topic branch

- 7e02f7c188a Add phpcs rule for strict types on test modules
- f98517b9649 Require strict types across test modules
- f56a6768559 Fix exception on RedirectOnExceptionTest
- 6a34dd36f22 Fix UncaughtExceptionTest
- 3adb077ef52 No point in return after throwing an exception
- b06bc731952 Fix errors triggered from SessionTest
- 32688b5d85e Fix errors from FormTest
- 9ddfbea0ea7 Fix error thrown from SecurityAdvisoryTest
- b0eacbada1d Fix error thrown from RenderArrayNonHtmlSubscriberTest
- 5a9015ada2b Fix error thrown from AlertsJsonFeedTest
- 8982568c4e1 CS fix
- 959101d7958 A bit more error handling for WebDriverCurlService
- a175cb41347 One less nesting level on WebDriverCurlService added error handling
- 8266166b3d0 Get the curl error
- 57972a66c5f Fix error thrown from BlockFormInBlockTest
- 63d4287668a Fix errors thrown from EarlyRenderingControllerTest

@mstrelan, thanks for the feedback.

I will try to take a look at the issue you mention for the next iteration, I did not see your comment until I was about to reply to the issue 😅.

I also found that for a lot of issues here we need to look at the HTML output to see an error message, as it the underlying error won't present itself in the test output.

Indeed, the plan is to identify the tests set with errors, then run them locally, and use the actual test error output URLs produced on the test run as needed, and fix the error.

I opened new MR #5790, intended to point to errors to fix.
As mentioned on its description, the plan is to rebase it to remove the first commit, so that can be done later at 📌 Add declare(strict_types=1) to all test modules Postponed .

New commits on new `3402007-strict-types-test-modules-testonly` topic branch and related new PR #

- 4b73ae24c5 Declare strict types on test modules
- a5b8e85dbd Fix exception on RedirectOnExceptionTest
- 08643a6c68 Fix UncaughtExceptionTest
- 00def53870 No point in return after throwing an exception

The next step is to continue fixing the errors pointed by gitlab CI.

marvil07 → made their first commit to this issue’s fork.

@Wim Leers, thanks, I appreciate the feedback 👍

Extending StreamWrapperUri constraint

I have extended the constraint to be more flexible, and then used its new option on the original case targeted in this issue.
More details on my reply there.

New commits on 3395585-validate-icon_base_uri topic branch and related MR #5675:

- e39bf90e76 Require read and write stream wrapper flags on StreamWrapperUriConstraintValidator
- 508ab9f27f Add class constraint class restriction
- e8985374c9 Introduce constraint options for StreamWrapperUri constraint plugin
- 4fa3708d7d Require readable and writable stream wrapper on icon_base_uri media.settings option
- c6c058865a Fix typo
- 7aed3751e6 Fix logic error on constraint validator

Uncovering new configuration validation errors

In an unexpected turn of events, as the last gitlab CI pipeline documents, many new places are now failing validation while trying to install media module 😅.

Looking a bit into them, I see that the public:// stream wrapper is not available on them.
I am wondering why is that the case.
The public stream wrapper is declared at stream_wrapper.public service on core/core.services.yml file, which is not really part of any module.

I may get back into this, but any clues will be highly welcomed.

@xjm, thanks for the thorough review 👍
I appreciate a lot that you used the suggestions feature in gitlab for the set of direct syntactic/semantic suggestion.

I see the related change mentioned about strict typing in trait at b70dfff449a278e2248f42dc5bd6623093c41e0e.
It is nice to see that pattern starting to make its way into core.

I have modified the topic branch to have clear type hints, along with declaring strict typing on the new traits.
I also added a few small changes here and there, trying to use a bit more the added traits.

I added a follow-up about the to-do item in the added code at ✨ API to disable bundle translation in one operation Active .

The following commits were added.

- 1f6db0a809 Add some extra type hints
- 64f7f64292 Improve syntax on multiple new docblocks
- e7d677b53c Improve two docblock parameter type docs
- 56a07e912b Get strict types declaration on traits from mainline
- 490f463719 Fix a couple LanguageTestTrait return values
- c6c4bf7d03 Use strict types on the new test traits
- 4e745b61fb Use static keyword to call new traits static methods
- 30a347274d Use create language trait method a bit more
- 5ed5cc94db Unify a couple of comments
- a7f5cbaeaf fix typo
- 05bc830064 Link follow-up on one-operation entity language disable

Back to NR now.

marvil07 → created an issue.

@phenaproxima, @Wim Leers, thanks for the review 👍

I added a couple of changes to the 3395585-validate-icon_base_uri topic branch and MR.

- 411fc720d5 Validate the stream wrapper uri is a string at the start
- 715ef4bc1f Extend test validation to cover more stream wrappers

On the read-only stream wrapper case, I think it still may work, please find details in my reply at gitlab, and above.

Back to NR.

@catch, thanks for the feedback!

This seems about right for the change to me. For one test it has very little impact at all, but if we do it for lots of tests (which we've been slowly doing) it will eventually make a difference.

The way I think about it 💡: we run the test suite a lot every day, so any improvement over time running will be multiplied by the number of times it is executed, which is a lot, and therefore the change likely makes sense.
Here there is around 15 seconds gained on every testsuite run, let us say 10s, which multiplied by the runs for every MR push on any issue fork, plus the actual commits on core, is likely a reasonable gain itself.

Not sure what that means for the loss of coverage here though [...]

@Wim Leers, I guess this is a question for you.
Please find the Approach section in comment #34 above, with some ideas on next steps.

@phenaproxima: added a change to avoid using StringTranslationTrait by removing added html tags.

@smustgrave, hey sorry for the noise here 😅

I have been trying to answer the question from Wim.

Measuring

What's missing from the issue summary: the actual speed-up of GitLab CI test runs?

On gitlab CI, it is not as trivial to find out.

Using the codebase at the point the code changes started is a good way to compare the running time.
That is what I have tried, opened a new MR without the changes (I needed to add one so pipelines are triggered, but it is just adding a comment), only for comparison.

Locally, as reported above 📌 Fix test performance of Drupal\system\Tests\Cache\PageCacheTagsIntegrationTest Needs work , I see a time change from 24s to 18s on the changed test running time.

We could compare the total running time, but I am guessing that may have lots of other variables that could influence the run.
E.g. test runners may change in time, the actually used ones can differ, the context they are in at the time of the run is important for performance measuring, etc.

Having all that in mind, I see a jump from 12 minutes 14 seconds to 7 minutes 24 seconds.
That is unlikely explained by the test change, see above in this comment that the jump on a not-that-fast local computer is of just 6s.

@catch, thanks a lot for pointing that out.
I was surprised that there is no download to explore in a text file 😅

Looking at the results of the two runs of testPageCacheTags:

- without the change, MR 5709, reports 24.35s, on page 7 at https://git.drupalcode.org/issue/drupal-2254209/-/pipelines/60135/test_r...
- with the change, MR 5625, reports 9.17s, on page 59 at https://git.drupalcode.org/project/drupal/-/pipelines/59193/test_report

Approach

@Wim Leers, re: #32:

If the idea is to test in a real scenario, and standard profile can represent that, we should document that the test is not expecting to be moved to a different profile.
Idem if the target profile is umami, but in that case it sounds way out of scope here.

I would suggest to actually use the topic branch, the test is not doing performance testing, it is ensuring right behavior.
That should not be affected by the context profile; but if that is the case, the test is likely wanted to be expanded to cover multiple scenarios, including with multiple profiles, and again that is likely out of scope of this issue.

marvil07 → changed the visibility of the branch 2254209-before to hidden.

@phenaproxima: thanks for the suggestions.
I have applied most of them directly.

The assertSame() change needed some extra context from translatable strings.

Back to NR.

@borisson_, @phenaproxima: thanks for the feedback 👍

I added the suggestions, and also added test coverage on this round.

BTW I started with a unit test, but then morphed into a kernel test, so I get the underlying service available.

New commits on 3395585-validate-icon_base_uri topic branch and related MR #5675

- 1275f421a1 Rephrase constraint message.
- 41f1d1dfca Use class name instead of machine name for service lookup
- b017bc580c Fix typo
- 9bb2b18701 Add test coverage for StreamWrapperUriConstraint
- 2f7dddc947 Here you go cspell

A few more modules may be removed from the install list, please find details at https://git.drupalcode.org/project/drupal/-/merge_requests/5625#note_238407 .

@smustgrave, changes look quite lean 👍.

After @quietone's suggestion on the last review round, I think the set of used modules in the test is the minimal set.

Locally, even if different and likely slower than gitlab runners, I see an improvement jump in test time for that test from 01:04.356 to 00:55.312.

Marking as RTBC.

Adding an extra tag, given the indirect improvement I noticed locally over the time to run for multiple of the changed tests.

We could validate if the string is a stream wrapper? that it starts with something in the shape of [public|private]://[something]?
Or can you just add a normal path as well?

@borisson_, that was an excellent discovery question 👍 .

In theory, a normal path can also be used.
The code at hook_requirements() provides logic for handling both a stream wrapper URI, and a local file system path.
Said that, I manually tested that, and it just does not work as expected.

In other words, yes, it seems like a stream wrapper URI is required, which means either (a) checking for it is worth, or (b) there is a bug for handling normal file system paths.
I am assuming (a) is the case.

New commits on new 3395585-validate-icon_base_uri topic branch and related new [MR #5675](https://git.drupalcode.org/project/drupal/-/merge_requests/5675) are the following.

- ed7e3057c2 Add a new StreamWrapperUri constraint plugin
- b338b94b1e Validate media.settings configuration icon_base_uri key for a valid stream wrapper URI
- 266daac448 Fix a few problem on the new constraint validator

I have changed the issue summary to include the new constraint as API change.
Also a related change record added for it at New config validation constraint: StreamWrapperUri → .

marvil07 → changed the visibility of the branch 3395616-add-validation-constraints to hidden.

marvil07 → changed the visibility of the branch 3395585-add-validation-constraints to hidden.

I manually tried to merge from mainline, i.e. 11.x, into the topic branch, no merge conflicts.

I also tried the other way around, merging the topic branch into mainline, and no merge conflict there either.

I am moving the issue back to its previous state, and the "no-needs-review-bot" tag to avoid the bot.

marvil07 → changed the visibility of the branch 3367151-return-type-on to hidden.

I agree the typehints are out of scope here and it looks like this should be a docs-only fix. But #11 only changes MediaSourceFieldConstraintsInterface whereas the MR also does change the docs in MediaSourceEntityConstraintsInterface. If this is correct then we should change both at once.

@longwave, that makes sense.
I changed the issue title accordingly too.

I suggest opening a new MR rather than using patch workflow.

I opened a new MR 4264 based on patch #11 and the similar change in the MR for other interface.

Marking the path on #11 as hidden, as well as the previous MR for easier review.

Please feel free to skip contribution credits for me here if needed, since this is a novice task.
I decided to go ahead since (i) there was no activity here for a couple of weeks, (ii) extracting the bits from both an MR and a patch sounds a bit less of the novice side, and (iii) improvements on DX are nice to have.

I triggered a re-try of the two failing jobs, Nightwatch and PHPUnit Functional Javascript, and they now passed.
Likely unrelated.

The added change here is quite focused, and it is only extending error handling, marking as RTBC.

@drumm, thanks!

Change now in MR #213, marking as NR.

marvil07 → created an issue.

The tests that make a form submission of that page as part of testing the actual form were identified and marked with a @covers annotation.

@joachim, thanks for mentioning that!
It was useful to be more confident on where to change things.
I only see now that you actually linked to 📌 Identify which browser tests should be running the language settings form and add a @covers to document it Fixed on comment #13 above.

Next steps are likely to continue wit the list above starting at FileOnTranslatedEntityTest.

I think I got to the end of the list!

$ git grep -e 'admin/config/regional/content-language' --and -e 'drupalGet' --name-only 
core/modules/content_translation/tests/src/Functional/ContentTranslationDisableSettingTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationEnableTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationSettingsTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationSyncImageTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationUISkipTest.php
core/modules/language/tests/src/Functional/EntityTypeWithoutLanguageFormTest.php
core/modules/language/tests/src/Functional/LanguageSelectorTranslatableTest.php

The missing set of matches are relevant to keep for the following reasons.

• ContentTranslationDisableSettingTest, ContentTranslationEnableTest, ContentTranslationSettingsTest, ContentTranslationUISkipTest: they are expected to use the UI, as identified on its covers annotation, added at 📌 Identify which browser tests should be running the language settings form and add a @covers to document it Fixed .
• EntityTypeWithoutLanguageFormTest and LanguageSelectorTranslatableTest are actually the same case, but they were not annotated. The first is checking hiding behavior of the form, and the second is checking the behavior of the setting page translated and when the content_translation module is not installed.
• ContentTranslationSyncImageTest is using the form to verify if translation synchronization has been saved correctly, and it is actually testing that feature, which is implemented on content_translation_form_language_content_settings_submit(), extending that is setup from \_content_translation_form_language_content_settings_form_alter, so I added a covers annotation instead, following the pattern from other cases like this.

Changes from the original point are not few, but highly related, so it likely made sense to follow @xjm suggestion from comment #12.

$ git diff --shortstat 6dfc925033..HEAD
 26 files changed, 244 insertions(+), 271 deletions(-)

Locally I have seen some performance improvements on most of the changed tests.
I did not made a fully performance review, but I have seen 3s improvements on multiple tests, which is great.

New commits on 3384936-use-lang-api-instead-of-form topic branch and related MR #5485 are the following.

- 8a461eeb03 Add a way to disable a given entity bundle translation from the language test trait
- 70d76a317c Use language translation test trait on ContentTranslationOperationsTest
- 08b4010835 Use disableBundleTranslation on EntityReferenceFieldTranslatedReferenceViewTest
- 0b1a618951 Use content translation test trait on FileOnTranslatedEntityTest
- b006797e17 Use content translation test trait on PrivateFileOnTranslatedEntityTest
- 4c178eff02 Use language test trait on LanguageSelectorTranslatableTest
- 091254e831 Use language test trait on MenuUiContentTranslationTest
- 8b6f4ecfe1 Use language test trait on NodeTranslationUITest
- 63124ebc88 Skip UI on node article translation setup for NodeTranslationUITest
- 18b99dbda8 Use content translation test trait on PathLanguageTest
- 283a4a7099 Use content translation test trait on DisplayFeedTranslationTest
- b56db49d11 Use language test trait on SearchMultilingualTest
- 3972484183 Use content translation test trait on PathWorkspacesTest
- 5bb538b3dd Use content translation test trait on RouteCachingLanguageTest
- f3cb974759 Use content translation test trait on ContentTranslationContextualLinksTest
- 2f650a931e Add covers pointing to content language settings form on EntityTypeWithoutLanguageFormTest
- 29faaea3f6 Add covers pointing to content language settings form on LanguageSelectorTranslatableTest
- bda937f13b Add covers pointing to relevant form alter on ContentTranslationSyncImageTest

At this point, this is ready for review, marking it accordingly on the issue status.

Just in case this is useful, there are three child issues marked as RTBC, that are waiting core maintainer action 😅.
Namely 📌 Add new `EntityBundleExists` constraint Fixed at 2023-10-10, 📌 Add validation constraints to image.settings Active at 2023-11-08, and 📌 Add validation constraints to dblog.settings RTBC at 2023-10-30.

@Gábor Hojtsy, it took a few iterations to get it right, but I think the current code in the merge request is now ready for inclusion.

At tests/modules/upgrade_status_test_error/fatal.php, I added a couple of comments, one for php linting, and the other for coding standards.
The linting one is the important, that avoids a lint fail, which is expected in that case.
To avoid it I declared that it should be only linted on versions lower than php 7, and given composer requires higher than php 7 indirectly via dependencies, it basically will not happen ever.

That last bit needed some changes on UpgradeStatusAnalyzeTest, since a few messages there contain the line number of the parse error.

Finally, the directory assumptions on test, as you mentioned on #3 and #4, needed a few changes.
To solve it, I am relying on the extension object related to the module where the tested template lives.

There are multiple suggestions from phpcs, phpstan, and stylint, but they are all optional, so I have not really dive into them.

Marking now as NR.

marvil07 → made their first commit to this issue’s fork.

Direction

I don't feel like #15 has been addressed here, it's adding a huge amount of API surface directly from Symfony, with no real indication that these are going to be used anywhere.

@catch, yes, the original intention here is to make general symfony validators available as constraint plugins, so they can be used; but they have not been used directly anywhere, at least yet.

What happens when Symfony removes one of these, changes the API etc., are we going to have to start to add bc layers ourselves? We've already got enough API churn from this component as it is.

That is fair to say, I appreciate this surfacing again and reminding the possible consequences.
I have not followed closely how symfony validator has impacted core, since it was added.

From what it is mentioned, it sounds like the possible impact can be more expensive than not exposing them by core.
Maybe the right solution for core is just to not expose them, or at least not all of them, and leave the rest for contrib modules like field_validation → .

Already there

There is already a subset of symfony constraint, or at least the related validators, that is exposed by core.
This issue is about exposing all the rest.

A static set added atDrupal\Core\Validation\ConstraintManager::registerDefinitions().

• Callback
• Blank
• NotBlank
• Email (extended at Drupal\Core\Validation\Plugin\Validation\Constraint\EmailConstraint
• Choice

And the set at core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/, excluding custom constraints directly extending Symfony\Component\Validator\Constraint.

• AllowedValuesConstraint, extends Choice; along with AllowedValuesConstraintValidator, that extends ChoiceValidator.
• CountConstraint, extends Count.
• EmailConstraint extends Email, and uses EmailValidator, as mentioned on the previous set.
• IsNullConstraint, extends IsNull, using a custom IsNullConstraintValidator on top of symfony one.
• LengthConstraint, extends Length.
• NotNullConstraint, extends NotNull, using a custom NotNullConstraintValidator.
• RangeConstraint, extends Range, using RangeConstraintValidator.
• RegexConstraint, extends Regex.
• UuidConstraint, extends Uuid.

In other words, currently it seems core exposes 13 symfony constraints to be used, of the 71 that symfony currently provides.

Next steps

Likely, a decision about what to do needs to be taken.

Taking into account the suggested upgrade problems from the near past as stated on #15, it sounds like the best approach may be to not take any action here, and close this issue.

That may avoid some of the possible burden of needing to add compatibility layers if upstream decides to change not trivially, or at least reduce it, since core already exposes 13 of the symfony validators, directly or indirectly.

I have started with the patch at #3 as the initial commit.

@xjm, I have started to follow the suggestion to use something separate to unify these groups of API calls.
Currently, it is using traits on the test namespace, but if enough useful and generalizable enough, it may become an actual new API surface.
Keeping it on the trait, since it may not be generic enough now.

I have been changing the code added on related issues, i.e. 📌 ContentTranslationLanguageChangeTest should perform setup tasks using API calls not form submissions Fixed , 📌 ContactLanguageTest should use API to set up language Fixed , 📌 PathContentModerationTest should use API to set up language Fixed , and 📌 MenuUiNodeTest should use API to set up language Fixed ; since those cases are not matching the grep.
I also included changes from 📌 ImageOnTranslatedEntityTest should use API to set up language Needs work , and then modified them to use the created traits.

New commits on new 3384936-use-lang-api-instead-of-form topic branch and related new MR #5485 created with the following commits.

- bf992c5553 #3384936-3: Use API to set up languages in ModerationFormTest test
- ec28e72357 Add a language trait helper
- 9709759d3b Start using the language trait on ModerationFormTest
- b01c84b5ad Add enable bundle translation helper to language trait
- 6317ec7a73 Add a content translation test trait helper, extending language test trait helper
- 589ae61595 Move content translation manager actions to the content translation test trait
- e0edafbf50 Use content translation test trait on MenuUiNodeTest
- 1560fb1410 Use content translation test trait on PathContentModerationTest
- 965cfbfcc1 Use language test trait on ContactLanguageTest
- 101a903ac4 Use content translation test trait on ContentTranslationLanguageChangeTest
- 799c1d49e5 Apply patch from #3385815-4 by vbouchet, ayush.khare: ImageOnTranslatedEntityTest should use API to set up language
- 3dffbc3a33 Use content translation test trait on ImageOnTranslatedEntityTest
- e41d0c318a Add a helper to set a field translation status on tests
- 42cf800493 Use new setFieldTranslatable method on ImageOnTranslatedEntityTest
- 0a757c5fee Use language test trait on ContentTranslationUntranslatableFieldsTest
- 95df2e0802 Fix phpcs issues on changed lines :sweat_smile:
- b3fe756f21 Use content translation test trait on ModerationContentTranslationTest
- 8dd7b2b5af Use content translation test trait on ModerationLocaleTest
- 3d2ae9633c Use content translation test trait on ContentTranslationContextualLinksTest
- 474a791068 Use content translation test trait on EntityReferenceFieldTranslatedReferenceViewTest
- aeec7bc391 Remove unused use statement

$ git diff --shortstat  11.x
 13 files changed, 165 insertions(+), 127 deletions(-)

Changes are growing, but they still feel similar.

Also, many of the grep matches are actually relevant to be using the UI, most of the set at core/modules/content_translation/tests/src/Functional/\*Test.php.
Here an alternative grep, via git.

$ git grep -e 'admin/config/regional/content-language' --and -e 'drupalGet' --name-only 
core/modules/content_translation/tests/src/Functional/ContentTranslationContextualLinksTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationDisableSettingTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationEnableTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationOperationsTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationSettingsTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationSyncImageTest.php
core/modules/content_translation/tests/src/Functional/ContentTranslationUISkipTest.php
core/modules/field/tests/src/Functional/EntityReference/EntityReferenceFieldTranslatedReferenceViewTest.php
core/modules/file/tests/src/Functional/FileOnTranslatedEntityTest.php
core/modules/file/tests/src/Functional/PrivateFileOnTranslatedEntityTest.php
core/modules/language/tests/src/Functional/EntityTypeWithoutLanguageFormTest.php
core/modules/language/tests/src/Functional/LanguageSelectorTranslatableTest.php
core/modules/menu_ui/tests/src/Functional/MenuUiContentTranslationTest.php
core/modules/node/tests/src/Functional/NodeTranslationUITest.php
core/modules/path/tests/src/Functional/PathLanguageTest.php
core/modules/views/tests/src/Functional/Plugin/DisplayFeedTranslationTest.php
core/modules/views/tests/src/Functional/SearchMultilingualTest.php
core/modules/workspaces/tests/src/Functional/PathWorkspacesTest.php
core/tests/Drupal/FunctionalTests/Routing/RouteCachingLanguageTest.php

The missing match at EntityReferenceFieldTranslatedReferenceViewTest is a disable translation case.
I have not yet figured out the right way TM to do it correctly via API, suggestions are welcomed.

Next steps are likely to continue wit the list above starting at FileOnTranslatedEntityTest.

marvil07 → made their first commit to this issue’s fork.

@smustgrave, @larowlan, @alexpott: thanks for the feedback.
@ankithashetty, thanks for the extra fix.

I have added some extra changes around the provided feedback.

- 0a0a1798b6 No operation if readme is not there
- 1b71aa71f6 Replace the URL path instead of rewriting the whole file
- 858815f640 Move new hook_update_N implementation to a new hook_post_update_NAME

Back to NR.

I started a new merge request, 5457, with a commit using patch from #33.

We should ensure that the file is writable. It is possible that the site has chosen to make the sync directory not writeable.

@alexpott, I added extra validation around this.
On the cannot-write case, nothing is done.

This still needs an update test.

@quietone, I added an update test.
Please notice that the update tests do not really write the config synchronization directory, as the normal install does.
So, instead, I am writing the readme file manually before running the updates.

The set of commits on the merge request are the following.

- ece085606a Issue #2600558 by a_thakur, vsujeetkumar: Wrong path generated in README.txt in sync directory
- 5c3f034701 Rename hook_update_N to be the second 10.2.0 system update
- 12e0cc171c Use a message on the hook_update_N
- b4efa18839 Validate if the readme file can be written
- 02c2c29dd3 Add an update test
- cecfd695f7 Fix indent

Back to NR.

Add Functional to the namespace of the example.

E.g. see Drupal core's core/modules/system/tests/src/Functional/Update/*.php tests

marvil07 → made their first commit to this issue’s fork.

It seems like a non-maintainer, I cannot change credits. :'-)
Suggested credit list for maintainer to change: JeroenT, munish.kumar, halth, jcnventura, suzymasri, Pooja Ganjage, and vijaycs85.

So I'm confused as to whether the extension needs to be enabled or not. If it does need to be enabled, the extension should be required in composer.json to warn the user that they don't yet meet the requirements.

Upon further investigation, it is required, so that requirement should be marked in the module's composer.json.

Yes, it sounds like a good idea to add the extension dependency.
Some reasoning below.

The current composer version declares a dependency on php 7.2 and halite library.

  "require": {
    "php": ">=7.2",
    "drupal/key": "^1.0",
    "drupal/encrypt": "^3.0",
    "paragonie/halite": "^4.1 || ^5.0"
  }

Then, halite declares on its composer file a dependency on paragonie/sodium_compat: ^1.17, which suggests ext-sodium for PHP 7.0+.
And yes, that is a suggests and not a requires, which is likely to allow both pre and post php 7 versions to work, since the pre-php7 version is suggesting ext-libsodium instead, which made sense at the time.

Said that, the sodium_requirements() implementation here and the composer file is requiring php 7.2+, so indeed ext-sodium is required.

Hence, adding the requirement to composer makes sense.
Marking as RTBC.

This issue is likely out of date.

The 8.x-1.0-beta1 release already declares compatibility with ^8 || ^9, and the 2.0.0-alpha1 → release declares compatibility with ^8 || ^9 || ^10.

Also, I think the changes here are part of 2.0.0-alpha1, so this issue can be considered fixed.
See git diff fbc38f4..d2d67b6, i.e. the MR point vs the 2.0.0-alpha1.

I am also adding some issue credits based on reading the whole thread.

Please re-open if I am missing anything here.

It took an extra bit to make the cspell change OK from phpcs perspective :-)
Now I see CI approving ;-)

Some of the dictionary changes look a bit suspect: dflydev, grasmash, phpowermove, psysh, robo look like the dictionary was not generated from a clean environment.

@longwave, indeed, those changes seem to be unrelated.
They may be related to recent changes on mainline, and there seems to be quite some changes there.

$ git log --oneline --since="2 months ago" 11.x -- core/misc/cspell/
7e75964e69 Issue #3395891 by quietone: Fix spelling for words in test modules
a10e0f8d1f Issue #3328741 by quietone, smustgrave, xjm, catch, alexpott, longwave: Add a dictionary for Drupal-specific words
3c1a449c37 Issue #3391788 by quietone, xjm: Fix spelling of function names in tests
0940ba003b Issue #3399123 by Spokje, longwave, smustgrave: Upgrade JavaScript dependencies to latest minor/patch releases
71f949cb0b Issue #3395913 by quietone: Correct spelling in Config tests
0852886ca3 Issue #3368102 by finnsky, Gauravvvv, smustgrave, lauriii, e0ipso, andypost, mrinalini9: Create new SDC component for Umami (umami common footer block)
c7e751a532 Issue #3379121 by quietone, longwave: Skip spellcheck for 20 nonsense words that are parts of hashes
41b777c6be Issue #3395871 by quietone: Correct spelling of words in test modules and dramallama
cb123cf554 Issue #3392425 by quietone, smustgrave: Fix variable name spelling in non-tests, part1
36ab456505 Issue #3389283 by quietone: Fix spelling of words only misspelled in tests, part 1
eafbd246a9 Issue #3394093 by quietone: Correct spelling of corefake
6118e0dc37 Issue #3389282 by quietone, smustgrave, alexpott: Fix spelling for words that are only misspelled  in comments
ae55070f94 Issue #3391268 by quietone: Fix spelling of words only misspelled in tests, part 4
b78ac3f08a Issue #3390955 by quietone, smustgrave: Fix spelling of words only misspelled in tests, part 3
a8f52aa253 Issue #3352459 by catch, Bhanu951, Wim Leers, smustgrave, Fabianx, heddn, alexpott: Add OpenTelemetry Application Performance Monitoring to core performance tests
987548947c Issue #3389286 by quietone: Fix spelling of words only misspelled in tests, part 2
dbb54acf6e Issue #3312072 by fjgarlin, penyaskito, markconroy, lauriii, smustgrave, ckrina, Spokje: Display category-related recipes when seeing a recipe full page
8224c90c08 Issue #3387339 by bbrala, longwave: Integrate codequality reports into Gitlab

I went on and revert the dictionary change, and merged mainline again, to be sure to be on the latest status.
Then, I ran the spell check, and it still passed.

$ cd core
$ yarn spellcheck:core
...
CSpell: Files checked: 15156, Issues found: 0 in 0 files

There are actually some valid additions, that are related to the newly added validators.
I reverted all additions to the dictionary, and the spell check did not pass, as expected.

$ cd core
$ yarn spellcheck:core
...
CSpell: Files checked: 15156, Issues found: 9 in 1 files

Looking into the details, it is all coming from the added core/lib/Drupal/Core/Validation/ConstraintManager.php.

$ yarn cspell -c .cspell.json --root .. "core/lib/Drupal/Core/Validation"
yarn run v1.22.19
$ core/node_modules/.bin/cspell -c .cspell.json --root .. 'core/lib/Drupal/Core/Validation'
 1/40 ./lib/Drupal/Core/Validation/Annotation/Constraint.php 299.41ms
 2/40 ./lib/Drupal/Core/Validation/BasicRecursiveValidatorFactory.php 19.78ms
 3/40 ./lib/Drupal/Core/Validation/ConstraintFactory.php 16.03ms
 4/40 ./lib/Drupal/Core/Validation/ConstraintManager.php 83.91ms X
./core/lib/Drupal/Core/Validation/ConstraintManager.php:26:45 - Unknown word (Issn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:31:45 - Unknown word (Luhn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:169:42 - Unknown word (IBAN)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:193:43 - Unknown word (Issn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:194:42 - Unknown word (ISSN)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:195:18 - Unknown word (Issn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:218:43 - Unknown word (Luhn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:219:42 - Unknown word (Luhn)
./core/lib/Drupal/Core/Validation/ConstraintManager.php:220:18 - Unknown word (Luhn)
 5/40 ./lib/Drupal/Core/Validation/ConstraintValidatorFactory.php 20.18ms
 6/40 ./lib/Drupal/Core/Validation/DrupalTranslator.php 35.70ms
 7/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraint.php 9.90ms
 8/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php 48.61ms
 9/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/CidrConstraint.php 12.12ms
10/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraint.php 32.69ms
11/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.php 9.25ms
12/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/CountConstraint.php 7.68ms
13/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/DivisibleByConstraint.php 7.41ms
14/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/EmailConstraint.php 6.89ms
15/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/EmailValidator.php 7.49ms
16/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/GreaterThanConstraint.php 7.71ms
17/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/GreaterThanOrEqualConstraint.php 10.27ms
18/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/IdenticalToConstraint.php 6.43ms
19/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ImageConstraint.php 16.00ms
20/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/IsNullConstraint.php 6.39ms
21/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/IsNullConstraintValidator.php 8.05ms
22/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LengthConstraint.php 11.36ms
23/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LessThanConstraint.php 7.69ms
24/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/LessThanOrEqualConstraint.php 15.47ms
25/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/NotEqualToConstraint.php 8.93ms
26/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/NotIdenticalToConstraint.php 6.50ms
27/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/NotNullConstraint.php 5.44ms
28/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/NotNullConstraintValidator.php 7.44ms
29/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraint.php 5.48ms
30/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.php 16.61ms
31/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/RangeConstraint.php 8.29ms
32/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/RangeConstraintValidator.php 14.08ms
33/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/RegexConstraint.php 8.47ms
34/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/TypeConstraint.php 6.32ms
35/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldConstraint.php 6.95ms
36/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php 28.14ms
37/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UuidConstraint.php 5.73ms
38/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraint.php 17.82ms
39/40 ./lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraintValidator.php 14.34ms
40/40 ./lib/Drupal/Core/Validation/TranslatorInterface.php 7.78ms
CSpell: Files checked: 40, Issues found: 9 in 1 files

I agree that ignoring the few instances on the one expected file using it makes sense.
So, I have pushed the following changes to the topic branch.

- a4cc65801f Revert "Update cspell dictionary"
- e44f62a124 Get mainline changes
- 830cae3bb9 Revert actual additions to the dictionary
- fb8d79efa0 Ignore spelling for three symfony validator specific words

Locally, after the changes it looks like the following.

$ cd core
$ yarn spellcheck:core
...
CSpell: Files checked: 15156, Issues found: 0 in 0 files

Let us see if the test run is OK too.

@Wim Leers, thanks for the feedback!

This issue is growing lines changed 😅, it seems like the extra validation is uncovering lots of places where configuration needs to be changed to be compatible with the new NoMarkup constraint.

One nit, and one important missing thing: test coverage

Incorporated your suggestion on the violation message.

On test coverage, that is still pending for the new validation constraint.
Said that, I see it being used across lots of tests ;-)

This round, I have been focused on trying to solve a subset of the test failures that the newly added configuration validation is triggering on existing tests, namely unit and kernel tests.
Basically, in many places, the randomString() method is being used to generate configuration values of type label across many tests.
That method can produce some characters that are stripped by strip_tags(), used behind the new NoMarkup constraint.
Instead randomName() should be used to generate strings that are intended to be of label configuration type.

New commits added to the merge request were the following.

- 5238db45a2 Apply 1 suggestion(s) to 1 file(s)
- 67279343f3 Use known filter format names on ValidatorsTest
- 50daa4efd3 Tweak documentation on random machine name method on test trait
- 7a6ff4b395 Expose name generation from Random component into random test related trait
- 3be9e310df Expose name generation on RandomGeneratorTrait
- 17f9235577 Use simple name generation for role name on UserCreationTrait
- 9b541f76e3 Use random name generation for BookPendingRevisionTest content type label
- 65ae3c4475 Handle extra validation error triggered on SimpleConfigValidationTest
- 4b014a1b3c Use random name generation for EntityAccessControlHandlerTest
- 0b9be9fac0 Use random name generation on EntityLanguageTestBase
- 4b580de001 Document SimpleConfigValidationTest::testSpecialCharacters() new parameter
- 6c90468755 Use name generation on EntityReferenceItemTest
- 5641798c63 Use random name generation on EntityReferenceSettingsTest
- 06a13129b4 Use random name generation on TranslationTest
- b84fa8d7b2 Use random name generation on MediaTypeCreationTrait
- 289104ff11 Use random name generation on FieldDiscoveryTest

Next steps

Likely, to continue to fix the failing tests.
Tests are understandably failing on configuration validation.

First, let us continue with the simple errors, as the last commits.

Second, let us dive into the migration test errors, throwing unserializable exceptions.
This likely similar to the one found at #3395616-8: Add validation constraints to image.settings → , fixed with commit 3efe72af45f211827a4bec599a84b50085ca5b7e.

Third, let us dive into functional test failing.
E.g. on BlockUiTest, I see the following exception thrown

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.stark_scriptalertxsssubjectscript with the following errors: 0 [settings.label] The value should not contain HTML markup in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 94 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php). 

That is exactly expected, since its previous step is trying to save <script>alert('XSS subject');</script> as the title of a block, so it is likely we want to change the handling of the exception there to handle the new possible validation error from configuration validation, and not from the form handling side only.

Fourth, let us add specific test coverage for the new constraint.

I merged in mainline at commit d797300b9ec841621a32d3efa2f84657ac46c012, so branch applies cleanly again.
One trivial merge conflict solved on cspell dictionary, union merge.
Back to NR.

marvil07 → made their first commit to this issue’s fork.

Conclusion: rather than solving this only for the ConfigurableLanguage config entity type, we should solve this for the label config schema type.

I am wondering if this was already handled as part of 📌 Add validation constraint to `type: label`: disallow multiple lines Fixed , which adds relevant lines to core label config data type.

$ git grep -h -A10 ^label core/config/schema/core.data_types.schema.yml
label:
  type: string
  label: 'Optional label'
  translatable: true
  constraints:
    Regex:
      # Forbid any kind of control character.
      # @see https://stackoverflow.com/a/66587087
      pattern: '/([^\PC])/u'
      match: false
      message: 'Labels are not allowed to span multiple lines or contain control characters.'

We may want to add an extra constraint to validate no HTML mark-up is present, to actually address the original report at the description.
For that drupal Html component may be enough, along with a custom constraint.

I opened a new merge request around it on top of the 2514688-label-config-validation-nomarkup branch, on the issue fork.
Let us see if tests run points somewhere.

marvil07 → made their first commit to this issue’s fork.

media.settings:icon_base_uri is described as the 'Full URI to a folder where the media icons will be installed' from the media module schema YAML file.

It is set by configuration install YAML to public://media-icons/generic.

On hook_install() basically the files at core/modules/media/images/icons/ get copied into the actual directory pointed by the value of the config key, i.e. the four images there.
These icons are shown for instance on the /admin/content/media page, when relevant for the media type.

From grepping the configuration key, there does not seem to be any UI where this configuration can actually be changed.

$ git grep icon_base_uri
core/modules/media/config/install/media.settings.yml:icon_base_uri: 'public://media-icons/generic'
core/modules/media/config/schema/media.schema.yml:    icon_base_uri:
core/modules/media/media.install:  $destination = \Drupal::config('media.settings')->get('icon_base_uri');
core/modules/media/src/Annotation/MediaSource.php:   * 'media.settings.icon_base_uri'. When using custom icons, make sure the
core/modules/media/src/Entity/Media.php:    return \Drupal::config('media.settings')->get('icon_base_uri') . '/' . $default_thumbnail_filename;
core/modules/media/src/MediaSourceBase.php:        return $this->configFactory->get('media.settings')->get('icon_base_uri') . '/' . $default_thumbnail_filename;
core/modules/media/src/Plugin/media/Source/File.php:    $icon_base = $this->configFactory->get('media.settings')->get('icon_base_uri');
core/modules/media/tests/fixtures/update/media.php:      'icon_base_uri' => 'public://media-icons/generic',
core/modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php:    $icon_base = \Drupal::config('media.settings')->get('icon_base_uri');

In other words, this configuration key can only be changed externally, e.g via drush, or custom php, drush cset media.settings icon_base_uri 'public://media-icons/generic2'.

The hook_requirements() code seems to be the best reference of what is expected from the value.
It basically creates the directory dynamically and makes sure it is a directory and writeable.

This means that the actual value here is not expected to be a writable directory until its first use.
Checking for that may end up in a false positive, since the code actually does create it.

Based on this, the only condition I can think of for this configuration key is just to be a string that can become a writable directory.
And that, by itself, may not be such a reasonable custom constraint to create :sweat_smile:

Hence, I can only think of using the PrimitiveType constraint check here, but that should already be happening 📌 Add validation constraints to taxonomy.settings Needs review .
It is not clear if one should be added, or maybe we want to change config_inspector code to realize that there is already primitive type validation on simple types in the schema?

Conclusion: There is not really a strong case for validation of the media.settings:icon_base_uri configuration key.
Suggestions welcomed.