🇩🇪Germany @simonbaese

The new version of Drupal Symfony Mailer Plus introduces many changes. We are waiting for a stable release before porting Symfony Mailer Queue.

If a maintainer of the Book module agrees, we can work towards porting the features over. However, it would require more work, as test coverage is currently not provided by the Custom Book Block module.

This might be a little confusing because the book module went from Drupal core to the contribution space. Additionally, a major update was introduced to the new contrib book module at the same time. Your composer output shows the correct version conflict. When updating to the ^2.0 version of the book module, you also need to update to the ^2.0 version of the custom book block module. Please find an overview of the compatible versions on the project page Custom Book Block → .

This was reviewed and tested in a project context outside of Drupal.org by @anna-d.

Thanks for pointing out these issues. The first should not have an impact on the custom book block. The second may have an impact. We will take a closer look once there is a stable 3.0 release. Feel free to test compatibility beforehand.

simonbaese → created an issue.

simonbaese → created an issue.

simonbaese → changed the visibility of the branch 3528807-entity-email to active.

simonbaese → changed the visibility of the branch 3528807-entity-email to hidden.

simonbaese → created an issue.

@robbertnl This is precisely what I described earlier. Unfortunately, the Drupal Symfony Mailer email object is overloaded in its responsibilities and is not easily serializable.

I am biting the bullet here in an attempt to carry over most of the properties that could be set during the email build phase. From a code perspective, this solution is not the most beautiful. But other solutions might be even uglier.

Can you please review the merge request and test the changes in your setup?

I was unable to reproduce the described faulty behaviour with the changes from 🐛 Sends emails and retries failures not working for custom module emails Active . Setting to fixed. Please use the 1.2.x development branch for testing. Reopen the issue if the problem persists.

@gbmapo Thanks again for the support. The changes will be part of the next minor release. Please use the 1.2.x development branch until then.

Thank you for the changes!

One more comment: We will start marking the SymfonyMailerQueueItem internal. I suspect that other issues will come up. Currently, changing the queue item is a breaking change, and we must tag a new minor release. I want to avoid that in the future.

The issues mentioned in previous comments remain. The idea behind the Symfony Mailer module is to utilize email builder and adjuster plugins to customize the email being sent. This is possible with the existing plugins or through custom plugins.

Yet, in theory, it is possible to manipulate the Email object before it is sent. For example, one could do the following in the business logic.

$email = $email_factory->newTypedEmail('test', 'test');
$email->setFrom('test@example.com');
$email->setTheme('test_theme');
$email->setVariables(['test' => 'variable']);
$email->send();

Just in this short snippet, we have three different cases:

1. setFrom() is part of the BaseEmailInterface. Yet, this call is not respected by the Symfony Mailer module. One should use the FromEmailAdjuster through configuration instead.
2. setTheme()<code> is part of the Symfony Mailer <code>EmailInterface. In theory, it may make sense to adjust the theme through code, and the Symfony Mailer module would respect it. Yet, it is better to use the ThemeEmailAdjuster through configuration instead.
3. There is no "native" email adjuster or builder to set variables. So, it makes sense to allow setting variables through code.

As previously mentioned, supporting all methods on the email object would require object serialization for the queue. Additionally, one would need to reconsider how emails are placed in the queue and then reinitialized once they are sent. I would refrain from making this change, as it is contrary to the intent of the Symfony Mailer module.

So, for now, let's add support for adding variables. Can you please review and test the merge request? Note that the email adjuster or builders may set variables. That is why we need to merge the variables.

Sorry, I can not make a recommendation here. Please do not execute command-line input in your Drupal cron job. This may have security implications. You can see the relevant implementation in Drush\Commands\core\DrushCommands::run(), though. Maybe you can implement or find a similar queue runner.

Could you please check whether the changes in 🐛 Sends emails and retries failures not working for custom module emails Active also resolve this issue?

@gbmapo Thank you for the detailed report and the additional findings from your investigation. This is very helpful to understand the problem.

The Symfony Mailer module alters the email parameters, not the Symfony Mailer Queue module. Yet, the queue worker sends the email through the processing of Symfony Mailer again. To circumvent problems with altered parameters, we now reinitialize the email in the queue worker with the original parameters.

Can you please check whether this solves your issues? Additionally, could you please review the code changes?

simonbaese → created an issue.

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.