🇩🇪Germany @simonbaese

Thanks. After further digging into the module's code I did eventually get it to work with this patch by setting the OpenID Connect login destination to "/user/login".

Setting the login redirect in OpenID Connect to /user/login should prevent setting the openid_connect_destination. In an unpatched installation, you can see that in OpenIDConnectSession::saveDestination(). Therefore, the change in this issue would also have no effect.

... using the login route path rather than the hard-coded "user/login" ...

You are right. The check whether the destination is the user login could be more verbose. This is not in the scope of this issue, though. After checking the prior logic in OpenIDConnectSession::saveDestination(), I adjusted the changes here.

I submitted a patch on the OpenID Connect issue for the site being in a subdirectory ...

After checking out the patch, I imagine it will be hard to get this in. The OpenID Connect module is not responsible for handling the "pure" destination query parameter. The changes are also not backward-compatible because the behavior of OpenIDConnectSession::saveDestination() changes. I see this as a commonly used extension point. We are also changing the behavior of that method.

Sorry, I am clearly not as deep into this issue as you are. I was taking the code in Redirect after Login as it is. Please apply the patch in your setup and try it out. We also use the combination of Redirect after Login, Anonymous Login and OpenID Connect, and it works fine.

The first approach sounds wrong. The CountryChangeEvent would fire on every request regardless of the user action. As you mentioned, I think it is better to change the block. You can extend the existing block or maybe even use hook_block_build_BASE_BLOCK_ID_alter (I haven't tried the hook).

Can you please open a "clean" merge request with the changes regarding the International option?

Yes, this could be added to the block configuration.

I also see a CountryChangeEvent in your branch. This probably would not make it into the module. The idea is that the current country is set very early during the request (see CountryEventSubscriber) and then is immutable. If you have sound reasoning for this, please describe the use case.

simonbaese → created an issue.

What is going on here? This is not a feature request. How are we going from a merge request that more or less changes two lines to a merge request with + 245 − 239 changes?

Please see the original reasoning from an earlier comment:

• $max_bundle_number = NULL enter this conditional (the user did not specify a number in configuration, therefore, "unlimited" bundles)
• $max_bundle_number > 0 enter this conditional (the user specified a maximum, therefore use the pager in query)
• $max_bundle_number === 0 do not enter the conditional (The user set zero, therefore, no bundles should be loaded)

@gbmapo Thanks for the question. Technically, this module does not use batches. It uses queues.

The feature you are asking for is the responsibility of the queue itself. However, the Symfony Mailer Queue module implements a queue worker independent of the actual queue implementation.

A queue worker only implements what should be done to process a single item. You can see this in the Drupal\Core\Queue\QueueWorkerInterface::processItem() method.

In the default setup, you would process the database queue with a cron job. The relevant processQueue() method can be found in Drupal\Core\Cron. Unfortunately, there is no option to limit the processed items here.

You may consult the implementation in Drush for limiting the processed items. You can use the command drush queue:run symfony_mailer_queue --limit=10 with the respective flag. The relevant code can be found in Drush\Commands\core\DrushCommands::run().

This feature most likely will not make it into this module. However, this module offers the option to "rate limit" the email sending with the "Wait time per item" configuration option. Maybe that suits your concerns.

Other options may exist when using more advanced queues such as RabbitMQ.

Feel free to follow up with more questions.

Thank you for the report. This is probably related to 🐛 Remove '__disable_customize__' parameters while queuing legacy emails Active . I will check it out soon once client work is less busy.

@trickfun Can you please add some more details about your installation and the exact issue your are facing?

@interactivex, we added a new release to resolve ✨ Allow configuration option to include admin pages in negotiation Active . Please update the module and check whether this helps.

As Anna mentioned, your language negotiation configuration is rather unusual. Please note that the order of the negotiation plugins is essential. So judging by the screen recording, the "Selected language" negotiation plugin will always fire before the "Language-country URL" plugin. Note that the "Selected language" plugin is usually used as a fallback and placed at the end of the negotiation chain.

Further, when the "Exclude admin pages" configuration option is enabled in the language-country negotiation configuration, one needs a subsequent negotiation plugin to perform proper path processing.

Do not hesitate to follow up on this issue.

@jamsilver Thank you very much for the detailed comments.

Making the fallback mechanism responsible for the partial matching is a good idea. Also, it cleans up the interfaces for the other services. The issue still needs work for testing the path utility service.

I agree with the first paragraph.

I want to maintain a clean interface for the path utility service. Essentially, other services should not make assumptions or have any logic to handle the prefix in the path. I know that the path handling could be more efficient. But having a clear separation here is more important than a couple of nanoseconds.

Please also stay "close" to the original implementation in the scope of this issue. The behaviour in the fallback mechanism changes quite a bit. All the changes were correct semantically, though. Further, as I understand the logic now, we should have a valid (matches custom prefix or default pattern) once we arrive in the CurrentCountry service.

Needs work because we need to test more methods in the PathUtility service.

It still requires some more work. We do not yet handle partial matches in the path prefix. Therefore, the fallback mechanism can not work. This is also why the tests are failing at the moment.

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

I am sorry for the delay. This issue is more complicated than expected because of the upstream architecture. If anyone has a good idea of how to approach this issue, please add your comments here.

Thanks for reporting the issue. I think the proposed change is not correct because the parameter is used to handle emails differently when sending them. See the comments in Drupal\symfony_mailer\Mailer::doSend().

I am confused, though, about how legacy emails are queued at all. This should only be the case when using the "Queue sending" email adjuster. Can you please post the steps to reproduce the issue? Or describe in more detail how you are sending legacy emails?

Unfortunately, we have to juggle versions after the Drupal core book module deprecation. Moving forward, please use version ^2.0 when using the contrib book module ^2.0.

I will mark this issue as fixed after the changes in 💬 Cannot install 1.1 with contrib Book v2 Active . Please use version ^2.0 when using the contrib book module ^2.0.

I will mark this issue as fixed after the changes in 💬 Cannot install 1.1 with contrib Book v2 Active . Please use version ^2.0 when using the contrib book module ^2.0.

Can you please test the MR in your project? I do not have a project meeting the described scenario.

I found out why my test project was not working. I assumed that when you set a custom project and ref also the _GITLAB_TEMPLATES_REPO/REF and therefore _CURL_TEMPLATES_REPO/REF would be set as well. Maybe this is something we can address in another issue or add documentation.

So, the issue with the script is resolved. We can continue with the questions from #31.

I guess that the variables in the curl URL are not resolved correctly. Not sure how to fix this, though. Maybe some fresh eyes can help.

And, can someone help out with the test failures in the merge request of this issue? Some changes might be out-of-scope for this issue.

This is a work in progress. There is an error in the phpunit-enable-coverage.sh script. Maybe someone can help.

Recent changes on the merge request:

• Introduced phpunit (coverage) job that needs to be triggered manually.
• Introduced variable OPT_IN_PHPUNIT_COVERAGE to enable the job mentioned above.
• Using the prepare-phpunit-xml.php script to provide coverage definition for contrib modules.
• Properly uploading coverage artifacts.

There are some open questions:

• How to properly merge PHPUnit arguments when using the coverage job? At the moment, it concats the PHPUNIT_COVERAGE_ARGS with PHPUNIT_EXTRA. This may lead to duplications, which result in PHPUnit warnings.
• How to enable Xdebug or PCOV properly? I see multiple variants in this issue, the comments and other contrib modules. Note that "line coverage, branch coverage, and path coverage data will be collected, processed, and reported. This requires a code coverage driver that supports path coverage. Path Coverage is currently only implemented by Xdebug."
• What are the correct coverage definitions in phpunit.xml? I saw some concerns in 🐛 #3480767 Causes break in behavior of tests configuration Active . Currently, it points to the project files in web/modules/custom/... and excludes tests folders.

@mondrake Thank you. That is a nice pointer. Indeed, turning off XDebug or using xdebug.mode=debug prevents the error. Do you think there is value in keeping this issue open? Maybe we can cross-reference and close it if there are other open issues.

Ok, apparently the kernel tests pass in the pipeline. Maybe this is a problem with DDEV? Could someone give a pointer to what the issue could be? I will investigate a little more and put this issue on postponed.

Please also let me know how to set up the test to observe the error in the CI pipelines.

If this is an error in the provided test, please change the issue type to "Support Request". In that case, we should add a change note to 🐛 Ensure post transaction callbacks are only at the end of the root Drupal transaction Fixed because it is unclear what needs to be adjusted.

simonbaese → created an issue.

I am also interested in this question. @TipiT How did you solve this problem?

Thank you @jonathan1055 for all the work. I especially like the changes mentioned in #50.

Can you please try \Drupal::service('rip.manager')->start(); in your implementation?

Can you use the batch implementation in RipBatch? I think, that would be more suitable than introducing a static method.

I see. I have no problem with shipping these changes.

The module addresses an issue in the upgrade path from Drupal 9 to Drupal 10. Is it possible to upgrade directly from 9 to 11?

This module may not be relevant for Drupal 11. I'm not sure if we should bump this. Can you give your opinion?

Some notes after looking at this:

• Recently, a script was introduced to massage the core phpunit.xml.dist, see 🐛 #3480767 Causes break in behavior of tests configuration Active . We can probably extend the logic here.
• I also have reservations regarding performance as mentioned in #27. Plus one for making it an additional manual job.
• We should add documentation for the different settings, limitations regarding concurrency and the coverage configuration.

If you are fine with it, I can continue with some changes in the existing merge request.

When using version 2.0.1 with the changes in MR !5 the favicon tags are not resolved correctly. After following the installation instructions the HTML output looks similar to the following:

<link rel="icon" href="<link rel=&quot;icon&quot; type=&quot;image/png&quot; href=&quot;favicon-48x48.png&quot; sizes=&quot;48x48&quot;/>">
<link rel="icon" href="<link rel=&quot;icon&quot; type=&quot;image/png&quot; href=&quot;favicon-192x192.png&quot; sizes=&quot;192x192&quot;/>">
<link rel="icon" href="<link rel=&quot;icon&quot; type=&quot;image/svg+xml&quot; href=&quot;favicon.svg&quot;/>">
<link rel="shortcut icon" href="<link rel=&quot;shortcut icon&quot; href=&quot;favicon.ico&quot;/>">
<link rel="apple-touch-icon" href="<link rel=&quot;apple-touch-icon&quot; sizes=&quot;167x167&quot; href=&quotapple-touch-icon-167x167.png&quot;/>">
<link rel="manifest" href="<link rel=&quot;manifest&quot; href=&quot;site.webmanifest&quot;/>">
<meta name="apple-mobile-web-app-title" content="<meta name=&quot;apple-mobile-web-app-title&quot; content=&quot;test&quot;/>">

Please see the attached patch where more of the attributes are detected via regex and passed on.

If you like, I can also adjust the existing merge request.

The proposed changes have been addressed in 📌 [PP-1] Code styling and PHPStan level 8 Active !

Only other thing I can think of is composer clear-cache.

Maybe it would be interesting to take the module ecosystem into account.

Not sure why this is happening. The 1.0.x branch does not have a composer dependency on drupal/book. Maybe you can require version 1.0.0-rc1 before the module had a composer.json file.

The version 1.0.x depends on the Drupal core book module. The core module has been deprecated in Drupal 10.3, please see The Book module is deprecated → . We tagged version 1.1.x to depend on the new contrib book module. You may run into conflicts because we also increased the minimum Drupal core version requirements before tagging a stable release of this module.

This module is not tagged as compatible with version 2 of the contrib book module. There is no stable release yet. I want to avoid playing catch-up with upstream changes. Please use version 1 of the book module.

This issue might also be related 📌 Tweak PHPStan config so paths are always correct and baseline is more usable Active .

Hello James, thanks for the proposed changes. I was holding back on this because I haven't looked into the effects to the dynamic page cache. The concern is that we produce significantly more cache variations because of the country prefix. Do you have thoughts on that?

Added changes for Drupal 11 compatibility. @jamsilver Can you give this another review? Afterward, I will merge and tag a new release.

Issues will be addressed in ✨ Compatibility Drupal 10.3^ Active . Closing this issue.

This feature is on the roadmap. See Development Plan on the module page. In general, I thought it would be good to use plugins for the prefix negotiation. I can imagine scenarios where it would make sense to chain the negotiation.

Do you mean the 2.0.0 version of the book module?

dan2k3k4 → credited simonbaese → .

Thanks for the report. I will take a look at it during contribution day at DrupalCon this Friday.

I would also recommend posting a template with this issue. I went with the following text.

If you are starting your journey with Artificial Intelligence in Drupal, we recommend exploring the AI module → as a great place to begin.

Let me know when you would like to make changes. I will close this issue after two weeks.

I added the module to the AI (Artificial Intelligence) ecosystem. Shouldn’t links be both ways though?

I checked the issue today. Building emails programmatically does not work correctly, as reported. Unfortunately, this applies not only to variables but also to other email properties. The issue stems from the Drupal Symfony Mailer overloading the responsibilities of the email object. Hence, the email object can not be serialized easily to pass data to the queue worker.

Probably, the only way forward is to carry all the build properties to the queue worker and reinstate them again. This approach is slightly ugly and I will sleep on it.

A workaround for now is to use an email builder plugin and delegate the assignment of variables to it.

Bumping to major priority because this bug affects all emails built in the described way.

Thanks for the report. I am traveling right now. Will be able to get to the issue next week.

The module is deprecated in core and will be removed. This is why we are switching to the contrib module.

Reminder

Also, the initial commit to this MR is quite informative: Initial commit. Maybe we can keep it in the back of our minds.

Just wanted to comment. Thanks for the compromise. Maybe I should have been more precise in the issue. I also intended to add a readme to guide contributors to Drupal CMS - not a readme for a release. I went on a similar journey as described in an earlier comment. I read about an issue, but instead of getting started smoothly, I had to spend more time than necessary to set up the project. It is funny that I only learned about the wiki through this issue.

First of all, good job on following through with all of the recommendations. Your work elevates the code quality drastically. Just a quick scan - not a full review.

1. You are using property promotion incorrectly. See, e.g., PHP 8: Constructor property promotion for reference.
2. Code styling: Use {@inheritdoc} or {@inheritDoc} consistently. In Drupal Core {@inheritdoc} is used.
3. Code styling: Use capitalized acronyms in comments. For example, use "API" and "ID".
4. Code styling: Follow Drupal coding standards regarding method comments, i.e., "Use a third person verb to start the summary of a class, interface, or method." See API documentation and comment standards → for reference.
5. If the PHPStan baseline is empty, it can be omitted.

simonbaese → created an issue.

Moving the issue for https://www.drupal.org/project/layout_paragraphs_iframe_modal → to the Drupal.org project ownership issue queue.

See 🐛 Content Snapshot not populated when installing from configuration Needs review for the export issue.

After fix:

simonbaese → created an issue.

Just do not make the error message configurable. If a website administrator wants to change the message, they can do so directly through interface translation.

Reminder ...

I have some concerns about this security advisory coverage application. I will not do a detailed code review here because I think some conceptual questions should be clarified for this module first. Every module that touches on access control should do so with proper care. Additionally, user expectations should be managed well because one may assume that the module does more than it does.

1. 1. The user may assume that when access to a URL is blocked, the content is not accessible. This is certainly not true because content may be reachable through another URL, e.g., via path alias. The configuration form may be a good place to communicate this.
2. 2. The KernelEvents::REQUEST event listener fires quite late. There are probably multiple ways this can go wrong where a language negotiator or path alias manager redirects the user before.
3. 3. The access control is not enforced when the `referer` header is set. But this header could be manipulated.
4. 4. The configuration form does not properly validate the entered URLs. Also, I guess that paths are expected here. But then, even the module name does not fit anymore.

I would advise against enabling security coverage for this module until the conceptual flaws are remediated.

It is a very well-written module. Looks pretty good already. I also appreciate that you have been using GitlabCI directly from the beginning. Here are some notes that I collected while looking through the code:

1. Why are you limiting the allowed entity types in abstract_email_validation_entity_bundle_field_info_alter()? Users may be confused about whether email validation works on the entity field if the entity type is not on the list.
2. In abstract_email_validation_custom_form_validate() you probably misnamed the $abstract_api_service variable. Should be renamed to, e.g., $email_validator.
3. The constructor comment in EmailValidator exceeds 80 characters.
4. There is a pattern throughout your code where you assume that array keys exist. For example, in EmailValidator::getQualityScoreStatus() you assume that the $response has the array key quality_score. However, this response comes from an external source, and the content may change. Then your code would cause a WSOD. This is not only a bad pattern for external content. You can make the same argument from a "method perspective". How can a method trust another method to deliver an array in the correct shape? (Answer: with static analysis, but you are not doing that.)
5. In EmailValidator::validate() you can get the $detailed_settings later when they are needed.
6. I would rename the EmailValidator::detailedValidation() method to passesDetailedValidation(). Also, the $response variable in that method is confusing - maybe use $pass.
7. I would rename the EmailValidator::checkBlacklistEmail() method to isBlacklistedEmail().
8. I would rename the EmailValidator::checkBlacklistDomain() method to isBlacklistedDomain().
9. All your methods in EmailValidator are public, but the only service method is validate(). It is good practice to define the interface of your service and only expose the necessary methods.
10. I would rename the AbstractApi service to AbstractApiClient.
11. Following the previous note, you are naming the injected service AbstractApiHandler in the EmailValidator. But this is not a handler. Use $abstractApiClient.
12. The service classes for the email validator and Abstract API client are currently in the folders Validate and Http. This is rather unusual. Please place them in the Service folder.
13. Why is the AbstractEmailValidationSettingsForm class final?
14. For the error messages in the form validation in AbstractEmailValidationSettingsForm, please look up the difference between "i.e." and "e.g.", and how to place the commas.
15. In AbstractEmailValidationConstraintValidator the PHP docs for the $emailValidatorService property is wrong. Also, just call it $emailValidator.
16. In AbstractEmailValidationConstraintValidator this might be a problem:
    

    $this->context->addViolation($this->t($custom_error_message, [
      '%email' => $field_value['value'],
    ]));
    

    I know the custom error message comes from the config form that only administrators can access. But this is against Drupal security practices.

17. I would rename AbstractApi::callAbstractApi() to, e.g., validateEmail().
18. You are inconsistently typing properties.
19. Please review your code and check comments for coding standards, especially "third person for function" comments. See, e.g., https://www.drupal.org/node/1354#functions → .
20. Consider using property promotion for your constructors. You may need to bump the core version requirements for this.
21. Consider adding return types to all methods.
22. Consider adding a PHPStan configuration file phpstan.neon for your GitlabCI. The module is already passing the static analysis on level 1. And you can easily get to level 5 with some minor tweaks.

Most of the comments here are related to code style and best practices. Only point 16 is relevant for security.

simonbaese → created an issue.

Bumbed version requirements to drupal/core: ^10.3 || ^11. A new minor version should be tagged for the module.

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

@mandclu Agreed.

Closing this issue. Please use version 1.1.x or later for Drupal 11 compatibility. The new version depends on the contrib module Book → instead of the deprecated book core module.

Closing this issue. Please use version 1.1.x or later for Drupal 11 compatibility. The new version depends on the contrib module Book → instead of the deprecated book core module.

The changes look good. Also, there is no problem installing with the new dependency.

Hello Mihail, I added more information to the module page. Please refer to this blog post by Spawning.

simonbaese → created an issue.

Change description

Change title

Applied suggestions by @nagy.balint and updated branch.

simonbaese → changed the visibility of the branch 3404209-missing-active-language-2 to hidden.

I am surprised that makes a difference. You see this pattern $translation = $entity->addTranslation($langcode, $entity->toArray()); quite often, even in Drupal core. Can you describe the issue with the und langcode a little better?

simonbaese → created an issue.