🇩🇪Germany @JoCowood

Hey, i am a colleague of sunlix. After we fixed the missing authorization_drupal_roles_entity_base_field_info hook, there was a follow up issue in the new update hook authorization_drupal_roles_update_8004.

Because a do-while-loop is used, under some circumstances the loop is run through once too often. The finished variable is calculated like this: $finished = ($sandbox['current'] - 1) / $sandbox['total'];. This makes it appear as if the additional loop pass is intended. Unfortunately, the additional loop pass results in a user with the ID ‘NULL’ being loaded.

My commit converts the do-while-loop into a while-loop, fixes the calculation for the finished variable and adds an initialisation for the new variable. I think, now we are ready for a review.

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

Yes, I completely agree with you @catch. We have an eye on that issue, too :-)

I am a colleague of harkonn → . For us, the new field_group version 3.6 fixes the problem, because it drops the dependency on modernizr with the issue 📌 Automated Drupal 11 compatibility fixes for field_group - Fixed PHPUnit on GitlabCI RTBC .

We were able to break the problem down to the fact that JS library definitions with the same weight lead to the following behavior:

1. the function AssetResolver::getJsAssets uses the function AssetResolver::getLibrariesToLoad to determine the list of libraries to be loaded in this request

2. using debug and the documentation, it becomes clear that the returned list does not have a stable sort order. This means that the order can vary per request.

3. in the further course of AssetResolver::getJsAssets, a small value is added to the later weight of the library, which depends on the position in the original list:

// Always add a tiny value to the weight, to conserve the insertion
// order.
$options['weight'] += count($javascript) / 30000;

4. as a result, the libraries that previously had the same value now have a slightly different value, which is a good idea at first. The whole thing becomes problematic if the original list now has different sequences in different requests and the final sorting is therefore not stable.

5. later in AssetResolver::getJsAssets, JsCollectionOptimizerLazy::optimize is called which calls JsCollectionGrouper::group at the very beginning. The division into groups is in turn dependent on the order of the input list, as the current entry is always compared with the previous entry. If an entry now jumps to a different position, this changes the groups and, in the worst case, the number of groups. (Please see the *example)

6. it can happen that a URL parameter is used in a subsequent request to access a js asset group (index) that does not even exist there.

7. broken

(*) An example.
The documentation for the grouping states the following

// Group file items if their 'preprocess' flag is TRUE.
// Help ensure maximum reuse of aggregate files by only grouping
// together items that share the same 'group' value.

If we now have the following sequence of JS assets for the first request
A (preprocess: true)
B: (preprocess: false)
C: (preprocess: true)
then we have three groups.

If we now have the following changed sequence in the subsequent request
A (preprocess: true)
C: (preprocess: true)
B: (preprocess: false)
we only have two groups.

Thank you for the hint AdamPS

Hey einarulfhednar,

I would be more then happy to fix this bug in 4.0.0-alpha3 but I need a review. Do you mind reviewing my latest patch from #5?

Once there is a d10 version, there is no reason for us to ask for co-maintainership. Then this ticket can be closed.

JoCowood → created an issue.

MR created

I removed the unrelated change of requirements in the issue fork and therefore in the MR. I furthermore checked phpstan for more necessary changes but there are none. Can be reviewed again.

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

Hi geek-merlin,

thank you for your reply. That you do the D10 port when we provide the changes is even a better solution. We do not tear ourselves for the co-maintainership, just wanted to offer our help since we often use the module ourselves.

We will provide a patch or fork in the D10 issue.

JoCowood → created an issue.

JoCowood → created an issue.

My colleague goldfit found a bug in my patch. The submit button gets invisible, too. You cannot save a disabled scheduler. This is not part of my changes but the original code. I fixed that in the new patch.

Hi einarulfhednar,

thank you for your merge request.

I think the solution is good but not perfect. If you add the condition, you cannot access the remaining config of the inactive schedule. If a user wants to disable the scheduling but keeping the interval, date values and so on, this is not possible. The user will loose the rest of the config.

What about setting the following patch? It sets checked based on the record and repairs the conditional invisible state of all following form fields, which was part of the code but broken.

I reviewed and tested the patch and committed it to the 4.x branch

Thank you for your issue fork. I created a new branch 2.x and merged your changes to this new branch.

Your changes to XslFormatter.php contained a bug. You switched getQuery() and accessCheck(), but no problem, I fixed that.

Can you please review and test my changes, set the issue to RTBC so that I can create a release?

I created a patch for 4.x based on the patch from #3255438: Broken Title on newsletter editions →

I just added a code comment to clarify the change and moved up the initialization of the title field one statement, because otherwise the existing code comment would not be correct.

This is ready for a review.

I created a patch based on SvenRyens changes in #3230858 → which were
- tugboat config
- code clean up
- documentation
- but not part of Drupal 9 compatibility

I created a patch.

JoCowood → created an issue.

JoCowood → created an issue.

JoCowood → created an issue.

JoCowood → created an issue.

We released 4.0.0-alpha1 a few minutes ago. Branch 4.x is based on 2.0.0-alpha1. The release contains all Drupal 9 compatibility fixes, which were missing in tag 2.0.0-alpha1. It also contains all Drupal 10 compatibility fixes. It does not contain anything more, because of the coding guidelines for new major releases → .

We decided to update core_version_requirement to "^9.3 || ^10" because of the compatibility to simplenews 4.x.

My colleague Goldfit did the neccessary compatibility fixes based on 3289677 📌 Automated Drupal 10 compatibility fixes Fixed and the non-code-clean-up changes from SvenRyen in 2.0.x development branch.

I did the review and test and there was a review and test by Bushra Shaikh in 3289677 📌 Automated Drupal 10 compatibility fixes Fixed . Sorry, that we left out the "RTBC" state.

We fixed the points 1 to 4 of the proposed solution. Point 5 will be moved to a new issues and will lead to a new release.

This issue duplicates [#3289677] 📌 Automated Drupal 10 compatibility fixes Fixed .

Thank you for your patch roshni27 and your review and test Bushra Shaikh. We included this in the work of the issue 3379235 🌱 Drupal 10 and Simplenews 4 compatibility Active .

We decided to update core_version_requirement to "^9.3 || ^10" because of the compatibility to simplenews 4.x.

Please see the other ticket for more details on the first Drupal 10 compatible release 4.0.0-alpha1.

Hey tvalimaa, I think your approach is very good. It contains only a few errors, which I have corrected. In addition, the method signature of mapEntityField has changed. I have adjusted that as well. Please excuse that I am more a fan of the snakecase :-)

/**
   * {@inheritdoc}
   */
  public function mapEntityField(ContentEntityInterface &$content, array $webform_element, FieldDefinitionInterface $field_definition, array $data = [], array $attributes = []) {
    $fieldId = $field_definition->getName();
    $paragraphFieldValue = $data[$fieldId];

    if (!empty($paragraphFieldValue)) {
      $paragraph = Paragraph::create([
        'type' => 'text',
        'field_text' => [
          'value' => $paragraphFieldValue,
          'format' => 'full_html'
        ],
      ]);
      $paragraph->save();

      $fieldValue[] = [
        'target_id' => $paragraph->id(),
        'target_revision_id' => $paragraph->getRevisionId(),
      ];
      $content->set($fieldId, $fieldValue);
    }
  }

Can someone please review this and set the issue to "Reviewed & tested by the community"?

JoCowood → created an issue.

Based on the patch from #7 I developed a patch which uses dependency injection instead of the static Drupal::languageManager() call. Since Tenguiz has found the actual solution to the problem please give credit to him (also?).

Any chances, that we get a 2.0.0-beta1 version? If none of the maintainer has time for this module, is there a chance to get a co-maintainership?

It turned out that the quick and dirty solution is also a good and clean solution. Here is a patch for conflict:2.0-alpha3

I have debugged this problem together with a colleague, as we also want to use conflict together with scheduler.

We are pretty sure that this added line of code causes the problem:
https://git.drupalcode.org/project/conflict/-/commit/d1958eca3852ef55c87...

The key/value pair conflictWidget will be added to the value array of all fields. Later on the validation of the entity is triggered. Because of the additional information in the value (here data) array, the array is not empty and is considered complex (see [1]). For this reason the array entries are validated individually. Because the datetime information is still null, the NotNullValidator reports a validation.

Another negative side effect is, that the comparison of the local node and the server node in the conflict module always leads to the result "has changes". FieldComparatorDefault is the only available comparator, which does a simple equals comparison. This comparison is always false, because there is this additional entry conflictWidget.

Proposed solution:

A) Place the conflictWidget information at a different location

B) Add a tidy up function, which is called, when conflict finished its job. We did a quick and dirty try at the end of \Drupal\conflict\Entity\ContentEntityConflictHandler::entityFormEntityBuilder, which seem to fix the bug.

foreach (array_keys($form_display->getComponents()) as $field_name) {
  if ($entity->getFieldDefinition($field_name)) {
    unset($entity->get($field_name)->conflictWidget);
  }
}

We are working on a patch, which we will provide soon.

[1] \Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validateNode L.163

Patch to fix the above mentioned issue.

JoCowood → created an issue.