🇧🇬Bulgaria @pfrenssen

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.

PHPUnit failures are due to 🐛 PHP 8.4: Implicitly marking parameter $parent as nullable is deprecated Active .

Seems like this is blocked on 2 issues :(
Postponing on 📌 RegistrationType enum cases not adhering to coding standards Active and 🐛 PHP 8.4: Implicitly marking parameter $parent as nullable is deprecated Active .

pfrenssen → created an issue.

PHPCS failures are due to 📌 RegistrationType enum cases not adhering to coding standards Active .

pfrenssen → created an issue.

Thanks very much for the offer.

We could really need some help with reviewing the open issues. I was onboarded as a co-maintainer and worked on a number of issues in the second half of 2024. I went through many tickets, reviewed and merged the ones that were ready and started the 3.x branch. Unfortunately the momentum has slowed down a lot because many tickets that are in "needs review" state are the ones that I worked on myself and I couldn't move them forwards.

Moving this to 3.x since this is D11 related.

Thanks for the report, this looks like exactly the kind of issue that the D11 OOP hook system is designed to solve.

It would be good to add #[LegacyHook] to the old hook implementation so it is not executed twice. There is some more information about this in Support for object oriented hook implementations using autowired services → .

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

pfrenssen → created an issue.

Thanks very much!

pfrenssen → created an issue.

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

I was too quick in deciding to expose the metadata type directly as children of container / composite elements. It is actually better to expose WebformElement because there can be fields on them that are essential for rendering the forms in the frontend.

A good example is the "maxlength" for text fields. This is exposed on WebformElementTextBase which is an interface for WebformElement.

So I reverted ✨ Expose the element metadata directly in nested elements Active again in this MR.

This is looking great, thanks a lot! We can now configure PHPStan to not allow any failures and this is good to go.

I agree, Drupal 10.2 is no longer supported so we can drop it and set Drupal 10.3 as the minimum version.

I have done the first part of the work: I added support for generic composite elements. It looks like there are a few gotchas with composite elements:

The webform module expects the values for the child elements to be passed as parent_element_key[0][child_element_key] but then the validation messages are returned as parent_element_key[items][0][child_element_key]. This is going to be confusing for the GraphQL consumer that there is an unexpected [items] part appearing in the element key.

Another thing I noticed is that when a child element is required, and the composite element is configured to show N number of empty rows, that the validation errors returned by the Webform module are duplicated N times. I am guessing this is intended to make it easier to highlight the validation error in the Drupal rendered webform for the N empty rows. But for the GraphQL consumer this might be confusing since they might get more validation errors back than values they are submitting.

A third thing is that, since we don't know in advance how many child elements are going to be rendered for a composite field that allows multivalue child elements, the Webform module returns element keys like parent_element_key__child_element_key. This will need to be converted into parent_element_key[N][child_element_key] by the frontend implementation. We need to document this somehow.

I think in scope of this issue I am not going to try to transform the internally used logic for handling validation of these elements and just return what we get. Now we have some special cases when handling validation of composite elements, but there will probably be more edge cases in other elements. If this becomes problematic in the future we can implement a plugin system to handle these kinds of edge cases for each Webform element type.

Thanks so much for coming up with this smart workaround!

This is a great suggestion. The help text configuration in the Webform module consists of 3 configuration options which can be exposed as metadata fields:

metadata {
  help: String,
  helpTitle: String,
  helpDisplay: WebformHelpDisplayOption
}

enum WebformHelpDisplayOption {
  ELEMENT_AFTER
  ELEMENT_BEFORE
  TITLE_AFTER
  TITLE_BEFORE
}

I split off the removal of the nesting level to ✨ Expose the element metadata directly in nested elements Active . I will implement that first because it will simplify this issue.

pfrenssen → created an issue.

I started on this. It looks like we can also support the fallback element named WebformElementWebformAddress which works out of the box (without needing the Address module).

The address elements are so-called "composite" elements which share a lot of similarities to other elements like "Flexbox" and "Fieldset". We can unify these under a shared WebformElementContainerBase interface.

Now that ✨ Move properties on the WebformElement interface Active is in we can remove 1 level of nesting by directly exposing WebformElementMetadata on the child elements rather than exposing WebformElement which has become just a wrapper intended to slim down the schema.

Thanks!

pfrenssen → created an issue.

pfrenssen → created an issue.

This is looking great, thanks very much!

pfrenssen → created an issue.

pfrenssen → created an issue. See original summary → .

pfrenssen → created an issue.

Thanks for the feedback @dulnan! I will implement this.

Unassigning, let's first figure out the best way to approach this.

The GraphQL specification actually required fields defined on an interface to be duplicated across all the types that implement the interface.

I mistakenly thought we could generate a schema like this:

interface WebformElement {
  key: String!
  title: String
  description: String
  multiple: Int
  required: WebformRequiredElement
  type: String!
}

interface WebformElementMultipleValuesBase {
  multipleValues: WebformElementMultipleValues
}

type WebformElementAddress implements WebformElement & WebformElementMultipleValuesBase {
}

type WebformElementCaptcha implements WebformElement {
}

But the GraphQL interface specification actually _requires_ the fields to be duplicated on the implementors, with the motivation that this makes it easier to declare covariant types.

So it quickly becomes bloated like this:

interface WebformElement {
  key: String!
  title: String
  description: String
  multiple: Int
  required: WebformRequiredElement
  type: String!
}

interface WebformElementMultipleValuesBase {
  multipleValues: WebformElementMultipleValues
}

type WebformElementAddress implements WebformElement & WebformElementMultipleValuesBase {
  key: String!
  title: String
  description: String
  multiple: Int
  required: WebformRequiredElement
  type: String!
  multipleValues: WebformElementMultipleValues
}

type WebformElementCaptcha implements WebformElement {
  key: String!
  title: String
  description: String
  multiple: Int
  required: WebformRequiredElement
  type: String!
}

Since we are still building out the schema it is easy to see how this can bloat the schema exponentially.

A good solution to limit this could be to define only a single element on an interface, for example:

interface WebformElement {
  metadata: WebformElementMetadata!
}

interface WebformElementMultipleValuesBase {
  multipleValues: WebformElementMultipleValues
}

type WebformElementMetadata {
  key: String!
  title: String
  description: String
  multiple: Int
  required: WebformRequiredElement
  type: String!
}

type WebformElementAddress implements WebformElement & WebformElementMultipleValuesBase {
  metadata: WebformElementMetadata!
  multipleValues: WebformElementMultipleValues
}

type WebformElementCaptcha implements WebformElement {
  metadata: WebformElementMetadata!
  type: String!
}

Which is incidentally how it was in version 2.x-alpha1, but this got flattened out as part of the change request ✨ Remove Metadata subfields Fixed .

It is probably a good idea to revisit this, and to reconsider adding back the metadata field to limit schema bloat.

We should also take other measures to limit the size of the schema. For example we should not expose all field types by default.

Starting on this.

Thanks @agunjan085 for picking this up! Just a heads up, this is an issue intended for the 2.x-dev branch, but the merge requests are still made for the old 8.x-1.x branch. Please make sure you are working on the right branch!

Unfortunately I do not have the right permissions to fix the default branch in Gitlab.

pfrenssen → created an issue.

pfrenssen → created an issue.

Thanks for the updates @VladimirAus, I am really happy with how this is shaping up. Not only is the PHP error solved in a clean way, but we also have a helpful exception that can be caught by calling code and gives useful information about the underlying problem.

I would like to remove the second test because I think it is unnecessary and would make future maintenance of the class harder. And we have a PHPStan warning. I left comments on the MR.