🇧🇬Bulgaria @pfrenssen

pfrenssen → created an issue.

pfrenssen → created an issue.

Added test.

We should also sort by priority in \Drupal\graphql\Plugin\SchemaExtensionPluginManager::getExtensions() which is used by non-configurable schema plugins.

Maybe a warning is most appropriate. If the index has been configured to use immediate indexing, then the site administrator expects this to happen and a warning should be issued if it didn't for some reason.

Test is green, back to needs review!

I needed this patch in order to write the test for 🐛 Cache collision when multiple servers are using the same schema plugin Active because it was affected by the missing schema errors. So we could consider that the test in that issue also covers this one.

In order to test this we need the fixed config schema definition from 🐛 Graphql server composable schema can't be installed in kernel tests Needs review , otherwise we get missing schema errors when we are loading the composable plugin.

pfrenssen → created an issue.

pfrenssen → created an issue. See original summary → .

Postponed until ✨ Allow to override extension by introducing plugin weights / priorities Active is in.

pfrenssen → created an issue.

pfrenssen → created an issue.

Thanks for the report! This is already fixed as part of 🐛 Fatal error when translating event series with modified event instances Active in the 3.x branch.

pfrenssen → created an issue.

MR !15 contains the simple one-line fix.

pfrenssen → created an issue.

The current MR !28 will redirect any request to a HTML login page but this will cause problems for requests that accept MIME types other than HTML (e.g. JSON:API / GraphQL / image styles / aggregated CSS files / streamed media / ...)

I think it would be safer to simply reload the page so it is served to the anonymous user. While redirecting a logged out user to the login form is valid in many use cases, it is better to leave this to specialized modules that are better suited to this task (like Redirect 403 to User Login).

pfrenssen → created an issue.

D11 compatible release has been made, this can be closed.

pfrenssen → created an issue.

I did a scan with the Upgrade Status module and indeed the change to the info file is the only thing needed to be D11 compatible. There are no incompatibilities in the code.

Looks good, code is already Drupal 11 compatible, only the info file needs to be updated.

I reviewed the patch and checked again with Upgrade Status. Indeed we only need to update the info file and make a new release. All code is already Drupal 11 compatible.

I think the version that drops Drupal <10.3 compatibility is much cleaner and will save us the work to remove this B/C layer in the future. If we choose this version we should create a new 3.0 branch.

Quick remark: I see in your example the field name is not capitalized correctly, it should be FieldItemTypeDateRange with a capital R.

Thanks for working on this! It is a fair point that we should promote as many people from the waitlist as possible. The solution with the making the promotion configurable sounds great!

Would it be possible to commit the changes to the MR so the tests can run and the changes can be reviewed?

I will also unassign from this since I am not actively working on it any more.

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 Active 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.