🇧🇬Bulgaria @pfrenssen

The patch on its own doesn't solve the problem of adding missing translations to the index. It only works in combination with a custom hook implementation like shown in the issue summary. Maybe that's something we want to add here as well?

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

Once we drop support for Drupal 8 and 9 we can use readonly properties which were introduced in PHP8.1, with this all injected dependencies can be made readonly and you can safeguard against unwanted writes to the properties. I.e. it prevents child classes from messing with the injected dependencies, and allows static analyzers to detect bugs where accidental writes are done to these properties.

Now the constructor looks like this:

  protected $entityTypeManager;

  public function __construct(
    ConfigFactoryInterface $config_factory,
    EntityTypeManagerInterface $entity_type_manager,
    protected TypedConfigManagerInterface $typedConfigManager
  ) {
    parent::__construct($config_factory, $typedConfigManager);
    $this->entityTypeManager = $entity_type_manager;
  }

In the future it can look like this (assuming the parent class signature doesn't change).

  public function __construct(
    ConfigFactoryInterface $config_factory,
    protected readonly EntityTypeManagerInterface $entityTypeManager,
    protected TypedConfigManagerInterface $typedConfigManager,
  ) {
    parent::__construct($config_factory, $typedConfigManager);
  }

This is looking good. My personal preference is with the original MR https://git.drupalcode.org/project/menu_breadcrumb/-/merge_requests/23 - this will make it easier to make the injected dependencies readonly in the future.

This has been long fixed. Maybe it would be good to get a new release out?

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

Excel Serialization is now D11 compatible, 📌 Drupal 11 compatability Active got committed!

pfrenssen → created an issue.

pfrenssen → created an issue.

pfrenssen → created an issue.

pfrenssen → created an issue.

@dustinleblanc, there is an example hook implementation in the issue summary.

Needs to be ported to the 3.x branch.

pfrenssen → created an issue.

This is for the unsupported 2.x branch. Closing in favor of 📌 Automated Drupal 11 compatibility fixes for config_patch_gitlab_api Needs review .

Duplicate of 📌 Automated Drupal 11 compatibility fixes for config_patch Needs review

This is already fixed in the latest dev release. Just scanned the latest code again with Upgrade Status and it is fully D11 compatible. Only thing left to do is to make a new release.

Looking great, also nice to see the coding standards check pass!

pfrenssen → created an issue.

pfrenssen → created an issue.

Thanks very much for the report and the review!

Thanks @trichers for the report, @chrisla for the feedback, and @owenbush for the fix!

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

@awolfey, no this is not critical. It is also not a bug, this is just how it was originally designed. There is a very clear warning shown to the user that the data loss will occur and they have to confirm it.

This is a known shortcoming. The groundwork for solving this has been laid in ✨ By updating a series, it deletes all the eventinstances and recreates them, which deletes all information stored on the instance. Active . There is also very interesting prior discussion in that issue.

There is also ✨ Provide an EventInstanceCreator plugin that does nothing Active which has a simple but very effective solution that can be implemented as a hook and is waiting for review.

I added a new MR with my proposed fix. @joaopauloc.dev does this also solve the problem for you?

Thanks for the report! I can replicate the error.

However it is not the right solution to start maintaining a potentially unlimited list of routes that might at some point in the future decide to compute the availability on a new or orphaned event instance.

What is the root of the problem is that the registration creation service expects a fully populated event instance (including referenced event series):

  /**
   * Set the event entities.
   *
   * @param \Drupal\recurring_events\Entity\EventInstance $event_instance
   *   The event instance.
   */
  public function setEventInstance(EventInstance $event_instance) {
    $this->eventInstance = $event_instance;
    $this->eventSeries = $event_instance->getEventSeries();
  }

We should here throw an \InvalidArgumentException if an event instance is passed that is missing an event series. And on the calling side we should probably avoid calling into ::setEventInstance() if the entity is new.

We had a website where the Paragraphs reference field was mistakenly set to translatable. The content editors had already entered and translated content. We used the following update hook to copy the latest revision of the translated content into the paragraphs. This doesn't solve content where a translated entity is referencing different paragraphs in different translations. I hope it might help someone running into the same situation.

<?php

/**
 * @file
 * Install, update and uninstall functions for My Custom Module.
 */

declare(strict_types=1);

/**
 * Migrate existing content to translated paragraphs.
 *
 * The article node type was initially configured with translatable references
 * to paragraphs. This is not supported by the Paragraphs module. Instead, we
 * have to reference paragraphs directly and translate the individual
 * paragraphs.
 *
 * @see https://www.drupal.org/node/2735121
 *
 * Unfortunately content editors have already created and translated articles
 * before this mistake was realized.
 *
 * This will store the latest revision of each translated reference as a
 * translated paragraph. In the existing data some of the translated articles no
 * longer have the same paragraphs as the default language. In these cases the
 * paragraphs will be skipped and a warning will be logged. We have to inform
 * the content editors to restore these translations manually.
 *
 * This migration is in an update hook rather than a post-update hook because
 * we need to run it before importing the configuration change that will
 * set the translation reference field to be untranslatable.
 */
function mymodule_update_11001() {
  // The affected entity type and bundle.
  $entity_type_id = 'node';
  $bundle = 'article';
  $field_name = 'field_paragraphs';

  // Temporarily enable the translatability of the paragraph reference field.
  // This will make sure we can access the previously created translations. Note
  // that we don't need to restore the original value of the field. This will be
  // taken care of by the configuration import.
  $config = \Drupal::configFactory()->getEditable('field.field.' . $entity_type_id . '.' . $bundle . '.' . $field_name);
  $config->set('translatable', TRUE)->save();

  // Load all affected entities.
  // @todo If a large number of entities are affected, consider using a batch
  //   operation.
  $entities = \Drupal::entityTypeManager()
    ->getStorage($entity_type_id)
    ->loadByProperties(['type' => 'default']);

  $warnings = [];
  foreach ($entities as $entity) {
    $default_langcode = $entity->language()->getId();
    // Loop over the translations of the entity and collect the paragraphs.
    $paragraphs = [];
    foreach ($entity->getTranslationLanguages() as $language) {
      $langcode = $language->getId();
      $translated_entity = $entity->getTranslation($langcode);
      foreach ($translated_entity->get($field_name)->referencedEntities() as $paragraph) {
        $paragraphs[$langcode][$paragraph->id()] = $paragraph;
      }
    }

    if (empty($paragraphs)) {
      continue;
    }

    // Check that the paragraphs are the same for all translations.
    $langcodes_to_translate = array_combine(array_keys($paragraphs), array_keys($paragraphs));
    unset($langcodes_to_translate[$default_langcode]);

    $default_language_ids = array_combine(array_keys($paragraphs[$default_langcode]), array_keys($paragraphs[$default_langcode]));
    foreach ($langcodes_to_translate as $langcode) {
      $paragraph_ids = array_keys($paragraphs[$langcode]);
      if ($paragraph_ids !== $default_language_ids) {
        // If any paragraphs do not exist in the translation, skip it an emit a
        // warning.
        $non_matching_ids = array_diff($default_language_ids, $paragraph_ids);
        foreach ($non_matching_ids as $non_matching_id) {
          $paragraph = $paragraphs[$default_langcode][$non_matching_id];
          $paragraph_bundle = $paragraph->bundle();
          $warnings[] = sprintf('Skipped paragraph %d (%s) for content %d (%s) because it does not exist in all translations', $non_matching_id, $paragraph_bundle, $entity->id(), $entity->label());

          unset($paragraphs[$default_langcode][$non_matching_id]);
          unset($default_language_ids[$non_matching_id]);
        }
      }
    }

    // For each paragraph:
    // 1. Retrieve the paragraph in the default language.
    // 2. Create a translation of the paragraph for each other language.
    foreach ($paragraphs[$default_langcode] as $default_language_paragraph) {
      foreach ($langcodes_to_translate as $langcode) {
        $translated_paragraph = $paragraphs[$langcode][$default_language_paragraph->id()];
        if ($translated_paragraph->hasTranslation($langcode)) {
          $translated_paragraph = $translated_paragraph->getTranslation($langcode);
        }
        if ($default_language_paragraph->hasTranslation($langcode)) {
          $default_language_paragraph->removeTranslation($langcode);
        }
        $default_language_paragraph->addTranslation($langcode, $translated_paragraph->toArray());
      }
      $default_language_paragraph->save();
    }
  }

  return implode("\n", $warnings);
}?>

Would it be a good idea to commit this on the stable 3.x branch as well?

The default capacity can be set in the registrant settings form:

And then when creating a new event series the values will be prefilled:

The update hook sets the default capacity to NULL so the functionality will remain unchanged for existing users.

pfrenssen → created an issue.

joelpittet → credited pfrenssen → .

I also hit this problem. It happens when an event instance is a relationship in a View. The Views field handler is written in a way that it expects the eventinstance entity to be the base entity for the view.

This happens also with the other fields (availability, waitlist and registration count).

We should use $this->getEntity($values) which will resolve the correct entity type, rather than accessing the raw data directly with $values->_entity.

joelpittet → credited pfrenssen → .

Added a test. Fixed another bug that was uncovered by the test: in EventInstances::computeValue() we were retrieving translations from event instances without checking that they exist, which results in an exception.

Since there is a minor B/C break in this, I will for the moment only merge this in the 3.x branch. If you think this is worthwhile to merge in the 2.x branch, please reopen and set status to "Patch (to be ported)".

pfrenssen → created an issue.

Yay green!

Pinning the version would be a workable temporary solution, but we would then still have to update the template later.

It looks like the failures are just because in the test fixes introduced in ✨ Drupal 11 compatibility fixes for webform Needs work we are not making a distinction between Drupal 11.0 and 11.1. Now tests are running on Drupal 11.0.5, but some of the tests are checking features introduced in 11.1 (like ✨ Continuation Add Views EntityReference filter to be available for all entity reference fields Needs work and 📌 Use easily recognizable date format RTBC )

I'm hoping that fixing this will turn the pipelines green.

Created a bug report in the Webform module about the PHP limit being ignored under certain circumstances: 🐛 Upload filesize limit doesn't properly account for PHP max upload setting Active .

Blocked by 📌 Make tests pass Active .

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

pfrenssen → created an issue.

The Webform module also has a limit on the upload size across all files in a form. This is controlled with the form_file_limit limit in the webform, and the global setting settings.default_form_file_limit.

This is not in scope of this issue but it would be great if we can adhere to this as well. I created a feature request for this: ✨ Respect the file limit Active .

pfrenssen → created an issue.

Thanks for starting this!

Unfortunately I am not doing any more work on the old 8.x-1.x branch. My agency has moved all their projects to GraphQL 4.x and as such I am only assigned to work on the 2.x branch which supports this version. In my personal time I prefer working on cool new stuff rather than maintaining legacy software ;)

Maybe one of the other maintainers is available to have a look?

Thanks for the work so far! I did an initial review, I found a few small issues.

Uncovered now that we enabled strict typing in 📌 Adopt strict typing Active .

pfrenssen → created an issue.

Tests are green! Ready to merge. Thanks!

You can always move an issue to Needs Review if you want someone to review it. But I am taking care of it now. I fixed the type bugs and am waiting for the test result. If this comes back green this is good to go and I will merge it. I'm pretty sure a bunch of typing bugs that are not yet covered by tests will surface very quickly, but these can be handled in followups.

Thanks very much for the contribution!

This is looking very good already. Thanks also for the documentation updates.

The goal of using strict types is to find typing bugs, so these failures are showing that we have some bugs that should be fixed. There are undoubtedly more bugs, which will start to surface once this is merged. For now I will tackle the ones that are found.

The composer build works again! The custom PHP CodeSniffer file also is working. It looks like 16 files are still missing the strict types declaration. And already a couple type bugs are detected in the PHPUnit tests :) That's great and the whole point of doing this.

Setting back to needs work, the remaining strict_type declarations need to be added, and the type bugs fixed so the tests are green again.

Thanks a lot for the work so far!

It's because of the unrelated changes to the composer files, these files shouldn't be changed.

Awesome, thanks so much for working on this. I see there is a test failure, can you have a look? It looks like the introduction of the constructor in WeeklyRecurringDateWidget needs to be adapted to match the parent class.

Thanks for working on this! The MR has been made against the 2.x branch, but it should be done against 3.0.x instead. Can you rebase it? If the error still persists I can have a look. Normally no changes are needed in composer files.

Awesome, thanks so much for working on this!

pfrenssen → created an issue.

We should add a phpcs.xml.dist file to the root of the project.

Can you try with this as the content of the file? I didn't test it but took it from another module.

<?xml version="1.0" encoding="UTF-8"?>
<!-- https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-drupal/using-coder-and-php_codesniffer-in-gitlab-ci -->
<ruleset name="recurring_events">
  <description>PHP CodeSniffer configuration for the Recurring Events Drupal module.</description>

  <rule ref="Drupal"/>

  <arg name="extensions" value="php,inc,module,install,info,test,profile,theme,css,js"/>
  <arg name="report-width" value="120"/>

  <!-- Use 's' to print the full sniff name in the report. -->
  <arg value="s"/>

  <!-- Require strict types declaration in every PHP file. -->
  <rule ref="SlevomatCodingStandard.TypeHints.DeclareStrictTypes.DeclareStrictTypesMissing">
    <severity>5</severity>
  </rule>

</ruleset>

No idea why the merging script decided to put my name first in the commit message :-/ I only changed one line while @sayan_k_dutta did ALL of the work.

Anyway thanks a lot for fixing this so quickly!

Looking great, 300+ lines of unnecessary code are removed! Thanks a lot for working on this!

Thanks for the suggestion, adding it to the list!

pfrenssen → created an issue.

Bot found a PHPStan regression, thank you bot! Fixed, back to NR.

pfrenssen → created an issue.

Looks like one instance was missed, in \Drupal\recurring_events_registration\RegistrationCreationService::registrationOpeningClosingTime():

        case 'scheduled':
          switch ($reg_type) {
            case RegistrationType::SERIES:
              // ...
              break;

            case 'instance': // <<< Needs to be replaced with the enum value
              // ...

Probably this is not really needed, when a new event series type is added a new registrant type is automatically created. I'm thinking now that for most regular use cases this will be sufficient. If advanced users would like to allow site builders to create standalone registrant types they can unlock the add form in their own custom module.

I'm going to close this, let's focus only on use cases that are interesting for 80% of users and leave advanced cases for custom modules.

Probably this is not really needed, when a new event series type is added a new registrant type is automatically created. I'm thinking now that for most regular use cases this will be sufficient. If advanced users would like to choose between multiple registrant types per event type they can add their own custom UI to handle it. This can be done by implementing regular form alter hooks.

I'm going to close this, let's focus only on use cases that are interesting for 80% of users and leave advanced cases for custom modules.

pfrenssen → created an issue.

If you can make an MR with the minimum necessary changes to get D11 compatibility while still retaining full backwards compatibility with Drupal 9.3+ then that is very welcome. But let's do that in a dedicated issue, this issue is specifically on future forward changes and does not affect 2.x.

I had some small remarks, but they are already fixed. Looking good, thanks!

pfrenssen → created an issue.

pfrenssen → created an issue.