🇩🇪Germany @simonbaese

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 #3397162: Tweak PHPStan config so paths are always correct and baseline is more usable → .

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.

Faced the same issue and came to the same solution.

The state may not be the correct destination for migration data. Especially, when importing large amounts of data this will conflict with the new cache collector for the state service → . It might be better to use the key-value storage.

RTBC +1

I would like to mention the new module Symfony Mailer Queue → as an alternative to queue sending of emails. We found that existing modules such as Queue Mail are hard to integrate with Drupal Symfony Mailer. The new module leverages the plugin system of Drupal Symfony Mailer and has similar mechanics and configuration compared to Queue Mail. Please see the module page for more details.

Wouldn't it make sense to validate both composer.json files? Although the generated one is not distributed with the module, it is still used to install the test environment.

The core book module will be deprecated with Drupal 10.3 and removed in Drupal 11. We have to create a new release which depends on the descendent contrib module Book → . Although, there is a 1.0.0 version tagged already there is still a lot of movement in the issue queue. Therefore, we will at least wait until Drupal 10.3 to work on a new version of the Custom Book Block module.

@smustgrave I reverted the deletion of the RenderCacheTest. Can you please make respective comments and add todos in the other issues. I am having a hard time understanding your last comment.

Then the mentioned issue should be solved first, because we can not fix this issue if not removing the test.

After aligning with @kristiaanvandeneynde the RenderCacheTest should be removed entirely. See related issue 🐛 UserRolesCacheContext can lead to poisoned cache returns for user 1 Active .

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

The ContentModerationTest made a distinction between the user 1 and an admin user. Therefore, all tests for rootUser were simply removed.

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

Will update the MR with the recent changes soon.

@Sandeep_k & @Kanchan Bhogade Please read the issue description carefully. This issue aims to fix a bug in the current implementation. It does not intend to extend or change the menu behaviour. The max bundle number setting is currently not used for user bundles. Please open another issue, if you like to change that and do not bloat this issue. Please describe the expected behaviour when posting screenshots. Especially the second set of screenshots it is not clear.

As already said, I understand the case for the "vanilla" state of the module. I am giving feedback that the module offers extendability and this extendability is disrupted in a minor release. If you are extending the functionality in a price history module, why not enforce the schema from there?

Thanks for the quick reply. I understand the motivation to prevent duplicates in the price list item table. But this approach may be too restrictive, since the PriceListItem is a fieldable entity and the query in PriceListRepository::loadItems() allows alterations.

We can refactor our implementation to satisy an extended PriceListItemStorageSchema. But users with less technical experience probably would struggle with this.

Can you please explain how to handle the same entry with different currencies? In our current implementation, we can not define a unique key with

'type',
'purchasable_entity',
'price_list_id',
'quantity'.

We would also need

'price__currency_code'.

We select which price is applicable for the current market with a hook on the commerce pricelist item query tag. Not sure if this use case is not accounted for or if our implementation approach is wrong.

If you do not want to change the behavior, could please introduce a hook to alter the key definition?

@omarlopesino I don't see how your patch addresses the original issue. If you found another problem, please do not extend the scope of this issue, but rather open a new one.

Resolved all threads. Please test your suggestions before pushing code to the branch. Please review.

Added script to amend core requirements with Drupal 11.

Other question: Now I wonder if we also should introduce the PHPStan analysis for ealier versions. The same argument that was made in the original issue description could also be made in reverse. During the development of a custom module, a new commit may introduces changes that are not compatible with earlier versions anymore.

It is a little bit unusual to support upstream patches that have not landed, yet. That is why I will change this issue from a bug report to a support request. Please see the attached patch to fix your issue. Theoretically, this could also be committed to the Custom Book Block code base. But, I have to think about this a little more - setting the issue as postponed. Please use the provided patch in the mean time.

simonbaese → created an issue.

Visibility is improved.

And here is a small demo.

Pushed work in progress to issue fork. Current approach:

• Use Select form element as base for new TagifySelect form element.
• Attach tagify to input HTML element with dropdown which copies options and default value from select HTML element.
• Update selected values via add and remove Tagify events.
• The value callback then is the same as for a select form element.

Some todos:

• Handle single value setting in form element. At the moment, multiple values setting is used.
• Check edge cases for no options, identical options text and identical options keys.
• Check hierarchical options.
• Check if JavaScript can be simplified.
• JavaScript linting.
• Add form element description.
• Work on form widget.
• Add tests.

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

I have to mention a related issue #2752859: LanguageNegotiators weights not respected when implementing LanguageSwitcherInterface → , which was closed as duplicate in favor of this issue. If this issue is closed as won't fix and the responsibility is handed to the settings.php configuration, it will be very hard to provide extended language switcher functionality in contrib without having users go through a cumbersome setup.

To not cause confusion here. This issue works on language_url negotiation. Sorting in LanguageNegotiator::getEnabledNegotiators() as originally proposed would have solved #2752859 as well. Now this issue shifted away from that approach, which leaves #2752859 unsolved.

Work is happening in Merge Request #4. The country block is already resolving the links correctly. More work needs to be done to set the active link classes correctly.

Related change record → went unnoticed because it was handled outside of the book.module namespace. Will update soon.