🇧🇬Bulgaria @pfrenssen

pfrenssen → changed the visibility of the branch 3435379-automated-drupal-11 to hidden.

pfrenssen → changed the visibility of the branch project-update-bot-only to hidden.

This looks good, the update to the info file is the only change needed.

This has been fixed a long time ago.

A big shout out to @Xano for suddenly coming out of the woodwork and adding me as a maintainer to the upstream commercie/currency library! I have merged the fix and issued a new release. Now our tests are all green, and this is ready for review again!

We can remove the big warning "Username save logic changed in 2.x" from the project page now that this is fixed.

We can remove the big warning "Username save logic changed in 2.x" from the project page now that this is fixed.

I'm impressed with the tremendous work that was done here, thanks @jeremy1606 and others! I updated the tests and locally they are all passing if I apply the patch from https://github.com/Commercie/currency/issues/8

I guess this is now postponed on https://github.com/Commercie/currency/issues/8 but the library seems to have been written by @Xano to support this module, and does not seem to be maintained any more.

We have a lot of deprecation warnings coming from one of our dependencies (commercie/currency). There is an upstream PR to fix these: https://github.com/Commercie/currency/issues/8

Let's not mark any tests as skipped. It is better to let them fail so we have good visibility on the work that is still needed. Marking tests as skipped gives a false sense of progress.

Since no Drupal 7 versions are still supported, this can be closed.

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

This has been fixed in the 3.0.x branch in 💬 Support youtube shorts 3.x Active . Let's make sure to include the unit test from that fix here too.

@chris dart I missed your contribution, thanks for starting the work on the D11 compatibility. However this is just a planning issue where we organize the bigger task of modernizing the code base. I think we might be ready now to start on the actual Drupal 11 compatibility. So far we were blocked on our dependencies (like the Field Inheritance module) not being ready. But we should do the work in separate issues and link them here.

Yeah I am guessing that users deciding to move to 3.x will probably be doing this as part of a D11 update. Thanks for the review!

The blockers have been solved!

Cool, all tests are passing both on PHP 8.3 and PHP 8.4.

Really nice to see this moving forward! I added a new blocker: 📌 Require PHP 8.3 in the 3.x branch Active

pfrenssen → created an issue.

Looks good, we only need to declare compatibility.

I did the remaining compatibility fixes, but it looks like this still needs more work. Some of the updates to the tests look like they might introduce new failures. I ran the tests and had a large number of warnings, errors and failures. A number of these probably are already happening in the main branch.

pfrenssen → created an issue.

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

pfrenssen → created an issue. See original summary → .

This has been committed a long time ago, and there have been no reports of any problems, so I guess it is OK to mark this as fixed. Thanks all!

Thanks, that is a very good suggestion. By extending the try-catch block we can rely on the exceptions, this is a better solution than filtering out NULL values. It will also catch any other incorrect data types.

pfrenssen → created an issue.

pfrenssen → created an issue.

This is already merged apparently.

I have used this patch with great success in several projects. It is great to delete some translations that were added by mistake due to inexperienced developers passing dynamic strings to $this->t().

Now, this is really helpful in these cases, but in _normal_ usage this issue shouldn't really arise. When using good quality stable modules, and custom code that has been peer-reviewed, in theory no "bad" source strings should be present. So I think this is solving an edge case and should not be included in core. It would be great to have this in a small contrib module though.

I am in favor of closing this as "Won't fix" and move it to contrib.

I reviewed the last commit from @lussoluca, it looks good, and thanks for improving the naming of the variables!

I cannot set this to RTBC since I worked on it, but I think the remarks have been addressed.

In the current code base the most common way to detect whether we are running on Windows is str_starts_with(PHP_OS, 'WIN'). Let's use this for consistency.

This has been fixed in release 1.0.9.

pfrenssen → created an issue.

pfrenssen → created an issue.

Here is a Drush script I used to fix the data corruption that occurs if paragraphs have been wrongly translated due to this bug. This is specific to my project in that it only considers that nodes will have paragraphs. Other projects might also have paragraphs on other entity types. It updates the database tables directly since it is not possible to update default translations through the entity API.

This is completely untested and just intended as a starting point. Do not run this in production ;)

use Drupal\node\Entity\Node;

$query = <<<MYSQL
  SELECT pi.id, pifd.langcode, pifd.default_langcode, pifd.parent_id, pifd.parent_type
  FROM paragraphs_item pi
  LEFT JOIN paragraphs_item_field_data pifd on pi.id = pifd.id
  WHERE pifd.parent_type IN ('node', 'paragraph')
MYSQL;

$connection = \Drupal::database();
$paragraphsData = $connection->query($query)->fetchAll();
// The data set is huge, create a lookup table for quick access.
$paragraphsLookup = [];
foreach ($paragraphsData as $i => $paragraphData) {
  $paragraphsLookup[$paragraphData->id] = $i;
}

$query = <<<MYSQL
  SELECT nid, langcode FROM node_field_data WHERE default_langcode = 1
MYSQL;
$nodeLangcodes = $connection->query($query)->fetchAllKeyed();

$foundInvalidParagraphs = [];

foreach ($paragraphsData as $paragraphData) {
  // Skip paragraphs that are not the default translation because they are not
  // affected.
  if (!$paragraphData->default_langcode) {
    continue;
  }
  $hostId = getHostId($paragraphData, $paragraphsData, $paragraphsLookup, $nodeLangcodes);
  if (!$hostId) {
    // Parent paragraph no longer exists, skip.
    continue;
  }
  $hostLangcode = $nodeLangcodes[$hostId] ?? NULL;
  if (!$hostLangcode) {
    // Node no longer exists, skip.
    continue;
  }
  // If the host language is not the same as the paragraph language, we have
  // an invalid translation.
  if ($paragraphData->langcode !== $hostLangcode) {
    $foundInvalidParagraphs[$hostId][] = $paragraphData->id;
  }
}

foreach ($foundInvalidParagraphs as $hostId => $paragraphIds) {
  $node = Node::load($hostId)->getUntranslated();
  $hostLangcode = $node->language()->getId();

  echo "\nProcessing node {$node->id()} - $hostLangcode ({$node->getTitle()})\n";
  foreach ($paragraphIds as $paragraphId) {
    echo "  - Setting default translation for paragraph $paragraphId to $hostLangcode\n";
    $queries = [
      <<<MYSQL
        UPDATE paragraphs_item_revision_field_data
        SET default_langcode = CASE WHEN langcode = :host_langcode THEN 1 ELSE 0 END
        WHERE id = :paragraph_id
      MYSQL,
      <<<MYSQL
        UPDATE paragraphs_item_field_data
        SET default_langcode = CASE WHEN langcode = :host_langcode THEN 1 ELSE 0 END
        WHERE id = :paragraph_id
      MYSQL,
    ];
    foreach ($queries as $query) {
      $connection->query($query, [
        ':host_langcode' => $hostLangcode,
        ':paragraph_id' => $paragraphId,
      ]);
    }
  }
}

function getHostId($paragraphData, &$paragraphsData, &$paragraphsLookup, &$nodeLangcodes): ?int {
  if ($paragraphData->parent_type === 'node') {
    return (int) $paragraphData->parent_id;
  }
  $parentIndex = $paragraphsLookup[$paragraphData->parent_id] ?? NULL;
  if (!$parentIndex) {
    // Parent paragraph no longer exists, return NULL.
    return NULL;
  }
  return getHostId($paragraphsData[$parentIndex], $paragraphsData, $paragraphsLookup, $nodeLangcodes);
}

plopesc → credited pfrenssen → .

Added tests and ported the fix also to the Stable widget. There is a failure but this is also affecting the main dev branch and unrelated to this issue.

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

Yay! Thanks very much!

Thanks for the review! I will address the remarks,

And finally we can introduce an event that can be used to alter the collection name.

When creating a collection, the name of the collection is part of the schema that is passed to the Typesense server, so we need to make sure the altered collection name is set on the schema.

Next up, we can improve and simplify the ::getCollectionName() method. It currently works like this:

  public function getCollectionName(?IndexInterface $index): ?string {
    if ($index == NULL) {
      return NULL;
    }

    try {
      /** @var \Drupal\search_api_typesense\Entity\TypesenseSchemaInterface $typesense_schema */
      $typesense_schema = $this->entityTypeManager
        ->getStorage('typesense_schema')
        ->load($index->id());

      if ($typesense_schema == NULL) {
        return NULL;
      }

      return $typesense_schema->id();
    }
    catch (InvalidPluginDefinitionException | PluginNotFoundException $e) {
      return NULL;
    }
  }

So basically the logic is:

2. Get the index ID.
3. Load the TypesenseSchema entity with the same ID.
4. If it exists, return the ID of the entity we just loaded.

But since both the index and the schema entities have exactly the same ID, this whole logic can be reduced to:

  public function getCollectionName(?IndexInterface $index): ?string {
    return $index?->id();
  }

I also think it doesn't make any sense to allow passing in NULL for $index, and we should also not allow NULL to be returned. So this can become:

  public function getCollectionName(IndexInterface $index): string {
    return $index->id();
  }

pfrenssen → created an issue.

Many methods in TypesenseClientInterface are taking a collection name as an argument, for example:

  /**
   * Removes a Typesense index.
   *
   * @param string|null $collection_name
   *   The name of the index to remove.
   */
  public function dropCollection(?string $collection_name): void;

In the current code base, since the collection name was always equal to the index ID, we were often passing in the index ID in lieu of the collection name, here is an example from SearchApiTypesenseBackend::removeIndex()

$this->getTypesenseClient()->dropCollection($index->id());

We should change this everywhere to use the collection name instead, so we properly decouple the collections from indexes. It will become:

$collection_name = $this->getCollectionName($index);
$this->getTypesenseClient()->dropCollection($collection_name);

In order to facilitate this, we should make SearchApiTypesenseBackend::getCollectionName() public so we can easily get the collection name for a given index.

In the current implementation the Search API index name is used when the Typesense collection is created. This has caused some confusion in the code base for interacting with the schema:

Using the index ID
Sometimes we are retrieving the schema using the index ID, example:

src/Controller/TypesenseIndexController.php
62:    $schema = $backend->getSchema($search_api_index->id())?->getSchema();

Using the collection name
Sometimes we are retrieving the schema using the collection name, example:

src/Plugin/search_api/backend/SearchApiTypesenseBackend.php
407:    $schema = $this->getSchema($collection);

Since in 100% of the cases, the collection name is derived from the index, let's standardize this on the index.

pfrenssen → created an issue. See original summary → .

Thanks for bumping this :) We are already D11 compliant for a year now, this can be closed.

Duplicate of 📌 Drupal 11 compatibility Active .

pfrenssen → created an issue.

Thanks for the review! I created a followup for the coding standards violations since these are out of scope, the violations are in files untouched by this MR. 📌 Fix coding standards violations Active

pfrenssen → created an issue.

pfrenssen → created an issue.

This is fixed for 2.x, but can still potentially be backported to the 1.x branch.

Thanks for working on this!

Instead of adding additional code to the form validate handler, let's instead mark the field as required in Registrant::baseFieldDefinitions() as was proposed in the issue summary.

By doing this we will inform Drupal that the field is required. Drupal will then take care of the form validation for us. Also it will do some other helpful things like marking the field as required, support entity validation etc.

We do not need to make any changes to the form, it will all be handled automatically :)

Thanks for the quick response. The attached MR fixes it for me. I am not 100% sure if the patch is omitting certain fields that are necessary for the CSV export since I am not using that feature. Please let me know if anything needs to be done.

pfrenssen → created an issue.

Maybe a better solution would be to remove Drupal 9.5 from the supported versions and require PHP 8. Both Drupal 9.5 and PHP 7.4 are unsupported so the maintainers' efforts are better spent elsewhere.

I made issues for our 4 plugin types:

• 📌 Use attributes to define SchemaExtension plugins Active
• 📌 Use attributes to define DataProducer plugins Active
• 📌 Use attributes to define PersistedQuery plugins Active
• 📌 Use attributes to define Schema plugins Active

Would it be OK to include these in the roadmap for the 5.x branch?

pfrenssen → created an issue.

pfrenssen → created an issue.

pfrenssen → created an issue.

pfrenssen → created an issue.

Now that we are planning the next major, would it be a good idea to get attribute based plugin definitions in? [#SchemaExtension()] rather than @SchemaExtension.

pfrenssen → changed the visibility of the branch 11.x to hidden.

pfrenssen → changed the visibility of the branch 1255092-url-should-return to hidden.

Hiding older MR since it is no longer relevant as per #1255092-66: url() should return / when asked for the URL of the frontpage → . Path alias module is optional.

The automatic merge wasn't triggered.

Fixed PHPStan error. Maybe we can set PHPStan to not allow failures?

Just updating the version strings is not sufficient. I did a check and the following issues also need to be addressed:

1. Class Drupal\mailgun\Commands\MailgunSanitizeCommands implements deprecated interface Drush\Drupal\Commands\sql\SanitizePluginInterface: use \Drush\Commands\sql\sanitize\SanitizePluginInterface instead.
2. MailgunMail.php, line 111: Call to deprecated method renderPlain(). Deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use Drupal\Core\Render\RendererInterface::renderInIsolation() instead.

Great news, thanks a lot!

@owenbush do you want to weigh in?

Thanks! Looks good to me. There are some test failures but this is because the module is not yet Drupal 11 compatible, that is unrelated to this issue.

Setting this to major since this is causing a test failure that is affecting all open issues for 3.0.x.