🇵🇪Peru @marvil07

Related test needed a message adjustment.
And given test was being changed, I added minimal test coverage for the new option.

Tangentially, lint errors were reduced a bit for files related to the changes.

New changes added to the 3427939-dom-str-replace-text-mode topic branch:

- 7e422bd Update related error message on the test case
- efa6cf1 Add minimal test coverage for text mode
- f725ace Reduce lint errors for DomStrReplaceTest
- a84e3e8 One less lint error on DomStrReplace

Merged mainline on the topic branch to solve the merge conflict:

- fdb0368 Get changes from mainline to solve merge conflict

heddn → credited marvil07 → .

heddn → credited marvil07 → .

@gabesullice, ℹ️ the changes on the MR may be impacted by the last merge I have added into mainline, namely MR !5 related to 🐛 Fix permissions Active .

@heddn, thanks for the extra MR here.

As mentioned there, I added some documentation changes, and then merged it in after testing it locally.

The one thing that may be missing here may be to add a way to override reviewer-only permission.
E.g. to add an Approve any workspace approval requests that allows a user to approve/reject any request.

That could be a follow-up, or a new MR here.

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

One note about the code in the MR. "Add to cart" does not select layout blocks. Only "Request translation" selects the layout blocks.

Indeed, that was the case.

I have added support for that.
Sadly I needed to do that with a work-around.
For some reason the form state value related to layout builder toggle is not available on the cart submit callback.
So, I have added a form state arbitrary value based on the actual value of the toggle, at the validation stage, which is the last I moment I see it there.

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

!6 got in, I added a merge commit, and solved related merge conflicts.

After that, I merged this in.

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

heddn → credited marvil07 → .

I'm wondering what needs-review-queue-bot is thinking :-)
I see pipelines in green: https://git.drupalcode.org/project/drupal/-/merge_requests/9595/pipelines

Back to NR.

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

I have added a hook_ENTITY_TYPE_delete() for both user and workspace entities.

On the user case, I am checking only for the reviewer.
I have not tried to check for the owner.
Arguably, even if the owner of the request approval is removed, the approval may still continue.

I also needed to add a small change to the entity class to handle the no subject case a bit more, following the recent addition to allow it to be nullable at a0d4ce4fab6a8310860a7de94936852a6198a1ec.

New commits on new 3478867-delete-orphaned-approvals topic branch and related new MR !2

- 260b87e Delete workspace request approvals when the reviewer is deleted
- e72f5e6 Handle no subject on WorkspaceApprovalRequest::urlRouteParameters()
- d9cc422 Delete workspace request approvals when the subject is deleted

marvil07 → created an issue.

dan2k3k4 → credited marvil07 → .

heddn → credited marvil07 → .

I think this is now ready to be reviewed again.
It has been a nice experience to get to know a bit more how performance tests are structured.

I have changed the setup phase to remove a views block at the start, that shows taxonmy terms on every pages, to have less noise.

I have also added a general cache bin remove method, to the main related trait for these tests.
It sounds like a great candidate to have there, since many tests will use it.

I also added a cool cache case, since it may also be relevant for performance tracking purposes.

I have not added a warm cache case, since I could not really think about an analogous set of steps for this case.
E.g. on node case it makes sense because there are sibling nodes, but for the term listing case there is not such a case.

Commits since last update on the 3476439-term-list-performance-test branch, and related MR !9595

9ad0e4e8f9 Actually look for things at the right place 😅
6bc0ea4a31 Make linter happy
709d5b6f2d Do not use the umami block with tags vocabulary terms as a block in the footer
19f8eb2d6e Unify the approach to match the term
a357447a30 Add cache bins clear to PerformanceTestTrait
2aef88e121 Add a cool cache case for vocabulary admin performance test

I have only added a hot/cold cases, no cool nor warm.

I'm not sure if that would apply.

Also, the listing is using Drupal\Core\Entity\EntityListBuilder, so it may be worth to generalize, or maybe that is more than we want to do now.

Feedback is highly welcomed.

Starting to try this one.

heddn → credited marvil07 → .

Current commit message format

There use to be documentation around commit message format, looking a bit around that use to be node nid 52287, but now that redirects to node nid 3156588, which is about issue credits.
AFAIK that format is the following.

Issue #[issue number] by [comma-separated usernames]: [Short summary of the change].

The current official drupal core hint about what the commit message format should be → , following Granting credit to issue contributors → is:

Commit the issue using the default commit message in the "Credit & committing" widget

That is currently part of drupal.org custom code that produces a template for maintainer, including core maintainers.
So a message like the example on the description is produced.

Issue #3420997 by sorlov, quietone, DanielVeza, smustgrave, alexpott, mstrelan: Convert MediaSource plugin discovery to attributes

The problem

The commit message format is not easy to scan to find the reason the change was made.

Yes and no.

Commit messages have historically been succinct.
The pattern in Drupal core development commit messages has been for almost all of its existence, especially since Drupal 5.x development around 2006, relying on drupal.org issues as the main way to have information around a given change.
The issue usually has most details, especially around decisions, approaches considered, and more.

So, for some of us it is just a reflex to go to the related issue for any commit we want to know more about.
Granted, that is a something that needs to be learned if it is the first time you look at drupal core git history.
That may not be the best developer experience, but the path is quite clear: go to the issue referred on the commit message.
Also, the issue is the first of second thing on the first line of the commit, so it is part of the git subject, shown on most of the tooling that reads git.

The other thing here, and likely the reason not directly highlighted on the issue description is that our current format implies listing all contributors of an issue, and that list can be long.
That is a practice that comes from times where we did not have issue credits, #2288727: [meta] Provide credit to organizations / customers who contribute to Drupal issues → , which is now the used solution.

Drupal is a really special project, in which it is the norm that multiple people help a given issue.
That is great, and helps in many ways.
In fact in the last year more than 79% of the commits involve 2 or more mentions vs. the rest being one mention; which usually means two, i.e. the author and the reviewer/commiter. See the end of the comment for the source of this affirmation.

If the amount of names, especially because they are before the actual description, is the actual problem that wants to be solved here, yes, moving that into git trailers is a good idea.

Instead if by "not easy to scan to find the reason the change was made" we think about self-containment of a given commit message, then we may need some extra steps.
Notice that other major software projects use self-contained commit messages, more on that below.

Self-containment of a commit message can help to actually read commit messages without looking externally to drupal.org issues.
That would be really great to have, but it would be a change in process.
Given the collaborative nature of the drupal community, building that summary as part of the issue process could be a good idea.
The simplest way is likely to use a new section on the issue summary description to do that.

On proposed solutions

All the existing alternatives proposed above use Conventional Commits 1.0.0 as its base, but they do not really address how we would like to embrace that in detail.
Yes, there is one example per option; but that is far from a clear guideline, especially about git trailers.

Also, for this to happen, the drupalorg project related code needs to be changed to provide the desired format.
Otherwise commiters will need to do the user formatting on git trailers manually.

Namely, what is missing would be to establish how to use \<type\>, [optional body], [optional scope], and [optional footer(s)].

We should decide on the set of types to use here, or a guideline on what to use.
E.g. embracing other project set or not.

The optional scope is also not clear.
The examples point to "media", which is the main module changed on drupal core git commit 7201d4bfba4aaedf5ff68ab9ccfa035554fa88be, from where the commit messag sample seems to come from.
The natural fit may be the core subsystem, as defined over MAINTAINERS.txt, but that may needs some standardization of names.

If we want to make the commit message more self-contained, then the body should be filled most of the time.

The optional footers also need to be defined, conventional commits point to a custom BREAKING CHANGE: \<description\> footer and then point to other git trailers, which mean any git trailer.
Examples suggest to use Authored-by: and Reviewed-by:.

The list of possible git trailers should be defined, so I guess that is reason enough to move this issue back to NW.

I would suggest to embrace other major project git trailers instead of creating new ones.
Both linux kernel and git projects use Reviewed-by:, but Authored-by: is not used.
For that linux kernel project use Co-developed-by:, and git uses Co-authored-by:.

Apart from those, we may want to define other possible trailers, and if we want to embrace or not BREAKING CHANGE: \<description\>.

To keep or not to keep issue number

I think this depends on what we want to do on commit message self-containment.

If we want to keep the existing approach to have most of the information on the issue, then the issue number should still be the first thing on the commit message.

If if we move to self-contained commit messages, in our current process, the issue discussion is still useful to reference, so keeping it at least as a git trailer is a good idea.

It may also be worth to point to a previous relevant comment around why Issue may still be useful:

Because prepending "Issue" adds some semantic value to the self-contained commit message (even if it's implied, it makes explicit the fact that the following number refers to an issue).

On some inspiration from other major projects

Other major projects could be a source of inspiration on how to do things.

The following links point to how the linux kernel and the git projects define their git trailers, along with more information about how they structure the commit messages.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#w...
https://git-scm.com/docs/SubmittingPatches#sign-off

A couple of examples of each of them follow.

netlink: specs: mptcp: fix port endianness from linux's commit 09a45a5553792bbf20beba0a1ac90b4692324d06

netlink: specs: mptcp: fix port endianness

The MPTCP port attribute is in host endianness, but was documented
as big-endian in the ynl specification.

Below are two examples from net/mptcp/pm_netlink.c showing that the
attribute is converted to/from host endianness for use with netlink.

Import from netlink:
  addr->port = htons(nla_get_u16(tb[MPTCP_PM_ADDR_ATTR_PORT]))

Export to netlink:
  nla_put_u16(skb, MPTCP_PM_ADDR_ATTR_PORT, ntohs(addr->port))

Where addr->port is defined as __be16.

No functional change intended.

Fixes: bc8aeb2045e2 ("Documentation: netlink: add a YAML spec for mptcp")
Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
Reviewed-by: Davide Caratti <dcaratti@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Link: https://patch.msgid.link/20240911091003.1112179-1-ast@fiberby.net
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

wrapper: introduce log2u() from git's commit d343068e4abc5e43d1ef1d5fed42bf4d7aa8cff4

wrapper: introduce log2u()

We have an implementation of a function that computes the log2 for an
integer. While we could instead use log2(3P), that involves floating
point numbers and is thus needlessly complex and inefficient.

We're about to add a second caller that wants to compute log2 for a
`size_t`. Let's thus move the function into "wrapper.h" such that it
becomes generally available.

While at it, tweak the implementation a bit:

  - The parameter is converted from `int` to `uintmax_t`. This
    conversion is safe to do in "bisect.c" because we already check that
    the argument is positive.

  - The return value is an `unsigned`. It cannot ever be negative, so it
    is pointless for it to be a signed integer.

  - Loop until `!n` instead of requiring that `n > 1` and then subtract
    1 from the result and add a special case for `!sz`. This helps
    compilers to generate more efficient code.

Compilers recognize the pattern of this function and optimize
accordingly. On GCC 14.2 x86_64:

    log2u(unsigned long):
            test    rdi, rdi
            je      .L3
            bsr     rax, rdi
            ret
    .L3:
            mov     eax, -1
            ret

Clang 18.1 does not yet recognize the pattern, but starts to do so on
Clang trunk x86_64. The code isn't quite as efficient as the one
generated by GCC, but still manages to optimize away the loop:

    log2u(unsigned long):
            test    rdi, rdi
            je      .LBB0_1
            shr     rdi
            bsr     rcx, rdi
            mov     eax, 127
            cmovne  rax, rcx
            xor     eax, -64
            add     eax, 65
            ret
    .LBB0_1:
            mov     eax, -1
            ret

The pattern is also recognized on other platforms like ARM64 GCC 14.2.0,
where we end up using `clz`:

    log2u(unsigned long):
            clz     x2, x0
            cmp     x0, 0
            mov     w1, 63
            sub     w0, w1, w2
            csinv   w0, w0, wzr, ne
            ret

Note that we have a similar function `fastlog2()` in the reftable code.
As that codebase is separate from the Git codebase we do not adapt it to
use the new function.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

In fact in the last year more than 79% of the commits involve 2 or more mentions vs. the rest being one mention; which usually means two, i.e. the author and the reviewer/commiter. See the end of the comment for the source of this affirmation.

To arrive there, the following was used as the base.

$ date -u
vie 13 set 2024 17:58:16 UTC
$ git describe
11.0.0-alpha1-541-g1c67d3b559
$ git log --format=%s --since="1 year ago"| wc -l
2141
$ git log --format=%s --since="1 year ago"| sed -e 's/:.*//' -e 's/^Issue #[[:digit:]]* by //'|awk -F, '{print NF}'| sort |uniq -c |sort -nr
    491 2
    440 1
    338 3
    247 4
    183 5
    116 6
     97 7
     56 8
     39 9
     23 11
     20 10
     14 12
     13 14
     10 13
      8 16
      7 17
      6 18
      4 21
      4 20
      3 24
      3 22
      3 19
      3 15
      2 29
      2 25
      1 82
      1 55
      1 52
      1 46
      1 35
      1 34
      1 32
      1 28
      1 26

P.S. It would be so nice to have <details> accepted on the comment format.

A more general version is now available on the topic branch.

New commits on 3470408-add-kc-user-delete-route topic branch, and related MR !279

- (rebased) 6b48c191 Add route to process keycloak user delete event relay
- 05d45550 Avoid full HTML response
- 739889ab Skip processing the queue if not enough data is present
- 78d42933 Refactor endpoint for Keycloak to be more generic

The problem describe on testing steps, and on the added failed test case, can be fixed with patch for core attached below.
Feedback about it is highly welcome.
Not entirely sure it is the right solution, since I have not looked at all possible cases for core.

I bump into this.
I reproduced it on a clean install, so it may be a problem on either group_content_menu or something similar to core's 🐛 Error: Call to a member function label() on null in Drupal\menu_link_content\Form\MenuLinkContentForm->form() (line 99 of /var/www/html/docroot/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php). Fixed .

Issue description filled with more details.

New commit on 3470408-add-kc-user-delete-route topic branch, and related new MR !279

89da64f8 Add route to process keycloak user delete event relay

Then, add it to a new drupalorg_keycloak_events_user_delete queue if it
is valid.

Finally, on queue processing, query KC one more time, and only remove
the user from Drupal if the user on KC does not really exist.

marvil07 → created an issue.

Change added!

New commits on new 3467929-use-hook-element-plugin-alter topic branch and related new MR !14.

- 6aa43cb Use hook_element_plugin_alter() now that it exist in core

marvil07 → created an issue.

hestenet → credited marvil07 → .

Added new MR https://git.drupalcode.org/project/drupalorg/-/merge_requests/275 with an extra option to use a local file instead of KC REST API.

Re-opening, for a follow-up, no need for extra issue.

For the issue summary update see #13, #14, #21 and #25.

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.