🇩🇪Germany @simonbaese

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.

Some more notes:

1. This is clearer now. Please change the variable name here (L229). $node = $items->getEntity() gets the entity this field belongs to. Later, after checking if this indeed is of NodeInterface, you can use $node->id() when resolving the Url.
2. Here (L217) you are loading the image style again, although you already have the image style.
3. Here (L213) you are using the variable name $images, but apparently these are files - at least you are using the FileInterface.
4. Here (L224) you can use $file->id() in case these are of FileInterface.
5. You can combine the two early returns in viewElements().
6. Use consistent quotes to wrap strings. There is a mixture of "..." and '...'. Probably best to go with '...'.

@gxleano There are two tasks open.

• Fix linting in JavaScript files (ESLint).
• Remove Drupal CI configuration from project. PHPUnit jobs can be done with Gitlab CI.

Fixing the ESLint should maybe be addressed in a separate issue, because there are many errors and this merge request may become too convoluted.

Open tasks:

• Add composer dependency for better_exposed_filters to have passing PHPStan analysis.
• Fix linting in style sheets.
• Fix linting in JavaScript files.

simonbaese → created an issue.

Postponed until ✨ Add the possibility to use with multiple select elements Active is resolved. We will need a select form element for facets.

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

Thanks for the report. The block is not integrated well, yet. It is the next task in our development plan.

LGTM!

Published a new release. Please try to replicate the problem with the new release.

simonbaese → created an issue.

Merging this since I would like to publish a new release soon. Please continue the discussion here if there are further issues. I will open a new issue for the active trail suggestion.

Merging this since I would like to publish a new release soon. Please continue the discussion here if there are further issues.

Merging this since I would like to publish a new release soon. Please continue the discussion here if there are further issues.

Thanks, looks good.

Thanks for investigating more. The changes in the mentioned issue should resolve this problem. I will publish another release during this week which will include these changes.

Bigpipe is now part of Drupal core. Lazy building works without Bigpipe. Bigpipe just changes the delivery strategy of the content.

Tested the block and the initial date and time do get cached. Therefore until the JavaScript is loaded the block will display cached values. When the user does not have JavaScript enabled (unlikely), the block will show wrong values. See this blog article on lazy builders which could be interesting in that context.

Some comments - this is not a full review.

1. Wouldn't it be nice to rely on the Drupal core date and time formats? Then this module does not need to worry about providing different formats and the users get more flexibility. Also, why not format the time and date entirely in the back end? This would make the JavaScript obsolete.
2. One could also rely on the users selected time zone or the default time zone of the site.
3. The PHPDoc of the TimeClockBlock constructor is incorrect.
4. Doesn't this block "suffer" from caching? Once the block is cached it would always display the cached date and time, correct?
5. The indentation in time-clock.html.twig is incorrect.

This is a partial review! There are more issues, but some general problems should be addressed first.

1. There are still many coding standards violations. For example, the indentation and camelCase vs. snake_case should be fixed.
2. The CommentOnTopController extends the ControllerBase. Therefore, the injection of the entity type manager and messenger is not necessary. Just use $this->entityTypeManager() and $this->messenger().
3. The field type stick_comment_on_top might be overkill for this purpose. Why not just use a boolean field? Also, it might be more suitable to add a base field for the Comment entity, rather than attaching a field. Has to be handled properly during install and uninstall though.
4. I may not understand correctly what is happening there, but the comment_on_top_preprocess_node() hook feels out of place. Maybe the module should not be responsible for altering the comment form.
5. The actual sorting of the comments happens in a view hook. What would happen if the comments are displayed through a different approach? I am not sure how to solve this, but maybe one could provide a service to sort the comments properly. Or maybe a custom storage could be implemented to sort the comments on load.
6. Also, the hook comment_on_top_views_pre_render() does not distinguish which view we are dealing with.

Some notes:

1. Declare dependencies on custom modules in info file as entity_reference_integrity:entity_reference_integrity and entity_reference_integrity_enforce:entity_reference_integrity_enforce.
2. Maybe this could move into package: Entity or package: Other.
3. The function prevent_entity_unpublish_form_validation_published_content() calls getformObject() on the form state multiple times. The method is getFormObject() though.
4. The conditional if (($type == 'node' || $type == 'user' || $type == 'taxonomy_term') ... should either be simplified or split to make it more readable.
5. The brackets in the statement $status = ($type == 'user') ? $status : $status['value']; are not necessary.
6. In SettingsForm the definition of $form['intro'] does not need prefix and suffix. This already is a markup element. You can use '#markup' => '<p>' . $this->t('...') . '</p>'.
7. In SettingsForm the tertiary operator in the default value of $form['enabled_entity_type_ids'] can be abbreviated as '#default_value' => $this->config('prevent_entity_unpublish.settings')->get('enabled_entity_type_ids') ?: [].
8. The EntityPreupdate maybe can be renamed. What this service does is validation.
9. The dependency injection in EntityPreupdate is done in an unusual way. It probably would be better to inject and use the config factory. Then the service does not need to implement the ContainerInjectionInterface and does not need to define the method create(). Also, the first argument of the constructor should be called $dependencyManager or better entityReferenceDependencyManager. Therefore, the property may needs to be renamed.
10. In my opinion, it is unusual to use the renderer to format a error message. Feels like shooting a fly with a cannon.
11. The variable $output in getEntityList() is redundant. Just return $this->renderer->render($build);.
12. The PHPDoc of the service states "Hook into entity update and throw an exception to prevent them disappearing." but the service does not throw an exception.
13. The PHPDoc for the constructor of the EntityPreupdate service is not correct.
14. The PHPDoc the renderer property in EntityPreupdate can be rewritten to match the style of the other properties.
15. The typehint for the renderer property should be @var \Drupal\Core\Render\RendererInterface. Notice the leading slash.

In general, I think it would be nicer to generalize this module to work with any kind of entity. At the moment, only node, taxonomy and user entities are supported. But especially for entity types such as paragraphs or commerce products this could be interesting.

Thanks for clarifying.

Some notes:

1. The info file states the dependency drupal:jquery_ui, but this module is not part of core since version 8.8. Also, is this dependency really necessary? If so, then it should also be declared in icm.libraries.yml.
2. The IcmFieldFormatter announces the implementation of the interface ContainerFactoryPluginInterface which is not necessary because the FormatterBase already implements it.
3. Consider using the factory injection pattern to avoid dependency on the parent class. See Using a Factory to Create Services.
4. The includes in IcmFieldFormatter should be sorted which is a coding standard in Drupal 10.
5. The comment for the aliasManager property in IcmFieldFormatter should be cleaned up. Also, this injection is probably not necessary. Just use Url::fromRoute('entity.node.canonical', ['node' => $file['nid'][0]['value']). Should be careful here when the option "content" is selected because the entity may not be a node. Then the nid array key may not exist.
6. In IcmFieldFormatter::settingsForm() the description for effects can be adjusted to "Should the image slider be vertical or horizontal?".
7. In IcmFieldFormatter::viewElements() the $file variable can be initiated later, because there are earlier returns following before it is used.
8. In IcmFieldFormatter::viewElements() you should check if $items implement EntityReferenceFieldItemListInterface before calling getEntitiesToView().
9. Also, getEntitiesToView() returns items of EntityInterface. Might be good to check or typehint that these implement the FileInterface before calling getFileUri() and createFileUrl().
10. Why is the cache max-age set to zero for the field formatter?
11. Use consistent quotes to wrap strings. There is a mixture of "..." and '...'. Probably best to go with '...'.

Could this be related to 📌 Missing translation - frontpage link Needs review ? The mentioned issue will be fixed with the next release. Just waiting for response there.

The problem is caused by the multi-value settings field for the swatches. It also saves empty values. Since the form is initialized with a input field for the "first" value, this empty value gets handed down to the widget.

simonbaese → created an issue.

This was caused by a mismatch of the field schema and the variable naming of the Coloris settings.

simonbaese → created an issue.

This was caused by strict comparison, because the field setting returns an integer zero.

simonbaese → created an issue.

simonbaese → created an issue.

simonbaese → created an issue.

simonbaese → created an issue.

The posted patch may conflict with the changes from another open issue ✨ Start level and active branch of tree Needs review !

It seems like you are using a patch from this issue ✨ Allow book navigation title to be overridden by top-level node title Postponed . The Custom Book Block did not play well with the changed parent configuration. Please use the changes from the merge request or the attached patch file and test again.

Unable to reproduce. Can you please provide more information?

• Which browser are you using? Does the error prevail in other browsers?
• Does the error show, when placing other blocks?
• What is the complete error AJAX error message?
• Does any related error show in the reports (/admin/reports/dblog)?
• Are there other modules that could cause this behavior?

Could be related to 🐛 View rendering multiple translations as the same translation (latest revised translation) RTBC .

Worked with patch #13 for a couple weeks now. Fixes the described issues.

Reviewed and tested. Works fine after changes.