🇬🇷Greece @idimopoulos

This creates the following for me:

Fatal error: Declaration of Drupal\ckeditor5\Plugin\Validation\Constraint\ToolbarItemDependencyConstraint::getRequiredOptions() must be compatible with Symfony\Component\Validator\Constraint::getRequiredOptions(): array in /var/www/html/web/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemDependencyConstraint.php on line 39

This is because the class version required by Drupal is different from the one required by mailjet. It is indeed not a good practice to force a package that is not used by core in a sense that, either you use composer to handle it globally (so, force Drupal to use the version of the package this module requires), or adapt to Core's functionality. This can break more things than it fixes :/

The functionality works great from what I tested. I only have 3 very small nitpicks.

I ended up creating https://www.drupal.org/project/drupal/issues/3494327 🐛 CKEditor5 should respect the autofocus attribute of a textarea Active for this issue as it seems a CKEditor issue rather than the private_message's module.
It might be that this can be closed (?).

Please, note, that since I as I said I am not an expert in UX, I have not provided any test waiting from some feedback for this.

idimopoulos → created an issue.

There is a bit more to this than meets the eye.
I "fixed" the initial request and then updated the InboxBlockTest to try and ensure that the thing works fine and is tested.

Unfortunately, the test would pass even without the "fix".
Looking into it through the selenium at port :7900, it seems that the test does not install the filter module, thus, a plain text appears in the default add message form in the tests.
The thing though, is that, the setting for autofocusing the message field, actually works fine. Because by default, it sets the focus to the field every time the DOM element is loaded (so it works with AJAX requests too out of the box).

On the other hand, if filter module is enabled, the field is hidden from display as the ckeditor5.js replaces it.
The problem is that focusing the `[autofocus]` element, does not really help because the cursor is not placed within the box ready to type a message.
Furthermore, by the time the new thread is inserted, the ckeditor JS is not triggered yet, thus we cannot simply try to focus the ck editor field.
From my browser console I was able to do it, so it is possible to focus, but not at the moment the thread is inserted.

Now, CKEditor does offer events to handle when that happens and you can do something like

  CKEditor5.on('instanceReady', (event, editor) => {
    // Set the editor to be focused when the CKEditor is ready.
    editor.editing.view.focus();
  });

Or even

  CKEditor5.on('instanceReady', (event, editor) => {
    // Set the editor to be focused when the CKEditor is ready.
    editor.editing.view.focus();
  });

but there are 2 problems:
1. The CKEditor as I noted, is launched after the private message JS. So we would need to do something about it afterwards.
2. CKEditor offers 2 events. On load and on instanceReady. The first is thrown when the library is ready and the second when ANY instance is ready. So I would assume that the second is the one we want. However, it is launched whenever ANY instance is ready. That means, that if for some reason, the site owner had a different editor somewhere in the page (let's say for example a contact form on the bottom of the page), we would not know which one to focus and IF we should re-focus on launch event.

The way I see it, we would need to have the private message manually initialize CKEditor5 IF it is enabled AND THEN focus it.

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

I have added a post update hook to clean the orphaned messages.
2 notes:
1) I used the Drupal API because the private message entity is fieldable. We cannot simply remove the lines from the main table.
2) I haven't provided a test for this. I used the ddev method to setup the project, installed it, used the following script to just create messages

<?php

use Drupal\Component\Utility\Random;

$random = new Random();

for ($i = 0; $i < 1000; $i++) {
    $message = \Drupal::entityTypeManager()
        ->getStorage('private_message')
        ->create([
            'owner' => 0,
            'message' => $random->name(),
        ]);
    $message->save();
}

and just ran the deploy hook to ensure everything is removed. Cheers

idimopoulos → created an issue.

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

Adding credits.

Requesting review.

Sorry, here is an updated script that works. I tested it in my case (I managed to replicate it by tampering with the data in the table)

<?php

use Drupal\Core\Entity\ContentEntityInterface;

$rdfMapper = \Drupal::getContainer()->get('rdf_sync.mapper');
$entityTypeManager = \Drupal::entityTypeManager();
$entityFieldManager = \Drupal::service('entity_field.manager');
$messages = [];
foreach ($entityTypeManager->getDefinitions() as $entityTypeId => $entityType) {
  if (!$entityType->entityClassImplements(ContentEntityInterface::class) || !($bundleEntityTypeId = $entityType->getBundleEntityType())) {
    continue;
  }

  $bundleStorage = $entityTypeManager->getStorage($bundleEntityTypeId);
  foreach ($bundleStorage->loadMultiple() as $bundleId => $bundle) {
    if ($rdfSyncSettings = $bundle->getThirdPartySettings('rdf_sync')) {
      // Get the base table of the entity.
      $baseTable = $entityType->getBaseTable();
      // Get the entity key for the ID.
      $idKey = $entityType->getKey('id');
      // Get the bundle key.
      $bundleIdKey = $entityType->getKey('bundle');

      // Load from the database IDs of the entity type that do not have do not
      // have a URI stored. Select IDs that in the `rdf_sync_uri` table, they
      // do not have a mapping to the columns `entity_type`, `entity_id`, and
      // `bundle`, or do, but the `uri` is empty.
      $query = \Drupal::database()->select($baseTable, 'e');
      $query->leftJoin('rdf_sync_uri', 'u', "e.{$idKey} = u.entity_id");
      $query->fields('e', [$idKey]);
      $query->condition("e.{$bundleIdKey}", $bundleId);
      $query->isNull('u.uri');
      $entityIds = $query->execute()->fetchCol();
      if ($entityIds) {
        $messages[] = "Entity type: $entityTypeId, bundle: $bundleId, IDs with missing URIs: " . implode(', ', $entityIds);
      }

      // Uncomment below to also appoint new URIs.
      foreach ($entityTypeManager->getStorage($entityTypeId)->loadMultiple($entityIds) as $entity) {
        $pluginId = $rdfMapper->getRdfUriPluginId($entityTypeId, $bundleId);
        $plugin = \Drupal::getContainer()->get('plugin.manager.rdf_sync.uri_generator')->createInstance($pluginId);
        $uri = $plugin->setEntity($entity)->generate();
        \Drupal::database()
          ->insert('rdf_sync_uri')
          ->fields(['entity_type', 'entity_id', 'uri', 'bundle'])
          ->values([
            'entity_type' => $entity->getEntityTypeId(),
            'entity_id' => $entity->id(),
            'uri' => $uri,
            'bundle' => $entity->bundle(),
          ])->execute();
        $messages[] = "Entity type: $entityTypeId, bundle: $bundleId, ID: {$entity->id()}, New URI appointed: $uri";
      }
    }
  }
}

print implode("\n", $messages);

We have discussed with Claudiu, I will merge MR 25 and will continue to https://www.drupal.org/project/rdf_sync/issues/3478179 ✨ Allow to check for the publication status before syncing Active to conclude the tests.

Hello @fishfree

From what I see, it seems that you might not have a URI for all entities. Probably you can check directly by running a quick query like

select * from node as n
left join rdf_sync_uri as r on n.nid = r.entity_id
where r.uri is NULL and n.type = 'my_bundle'

if we are talking about nodes and re run this for every bundle to see which IDs are empty.

Furthermore, I created the following script (works for me with nodes) to iterate through all entity types in order to check all of them for empty URIs. Sorry for all the container calls, it was created in haste. After running it with drush php:script ./my-script.php and you verify if I am correct, you can also uncomment the commented section designated, to also create URIs for the IDs that are missing them and update the entities. Please, note, that this will actually update the entities. If you do not wish them updated, adjust accordingly or manually force the values in the rdf_sync_uri table.

<?php

use Drupal\Core\Entity\ContentEntityInterface;

$rdfMapper = \Drupal::getContainer()->get('rdf_sync.mapper');
$entityTypeManager = \Drupal::entityTypeManager();
$entityFieldManager = \Drupal::service('entity_field.manager');
$messages = [];
foreach ($entityTypeManager->getDefinitions() as $entityTypeId => $entityType) {
  if (!$entityType->entityClassImplements(ContentEntityInterface::class) || !($bundleEntityTypeId = $entityType->getBundleEntityType())) {
    continue;
  }

  $bundleStorage = $entityTypeManager->getStorage($bundleEntityTypeId);
  foreach ($bundleStorage->loadMultiple() as $bundleId => $bundle) {
    if ($rdfSyncSettings = $bundle->getThirdPartySettings('rdf_sync')) {
      // Get the base table of the entity.
      $baseTable = $entityType->getBaseTable();
      // Get the entity key for the ID.
      $idKey = $entityType->getKey('id');
      // Get the bundle key.
      $bundleIdKey = $entityType->getKey('bundle');

      // Load from the database IDs of the entity type that do not have do not
      // have a URI stored. Select IDs that in the `rdf_sync_uri` table, they
      // do not have a mapping to the columns `entity_type`, `entity_id`, and
      // `bundle`, or do, but the `uri` is empty.
      $query = \Drupal::database()->select($baseTable, 'e');
      $query->leftJoin('rdf_sync_uri', 'u', "e.{$idKey} = u.entity_id");
      $query->fields('e', [$idKey]);
      $query->condition("e.{$bundleIdKey}", $bundleId);
      $query->isNull('u.uri');
      $entityIds = $query->execute()->fetchCol();
      if ($entityIds) {
        $messages[] = "Entity type: $entityTypeId, bundle: $bundleId, IDs with missing URIs: " . implode(', ', $entityIds);
      }

      // Uncomment below to also appoint new URIs.
//      foreach ($entityTypeManager->getStorage($entityTypeId)->loadMultiple($entityIds) as $entity) {
//        $pluginId = $rdfMapper->getRdfUriPluginId($entityTypeId, $bundleId);
//        $plugin = \Drupal::getContainer()->get('plugin.manager.rdf_sync.uri_generator')->createInstance($pluginId);
//        $uri = $plugin->setEntity($entity)->generate();
//        $fieldName = \Drupal::getContainer()->get('rdf_sync.mapper')->getRdfUriFieldName(entity: $entity);
//        $entity->get($fieldName)->setValue($uri);
//        $entity->save();
//        $messages[] = "Entity type: $entityTypeId, bundle: $bundleId, ID: {$entity->id()}, New URI appointed: $uri";
//      }
    }
  }
}

print implode("\n", $messages);

Further more, in https://www.drupal.org/project/rdf_sync/issues/3478179 ✨ Allow to check for the publication status before syncing Active , we are working on allowing a subscriber to tamper with the entities before syncing them. That means, that using this subscriber, and a simple check whether the entity does have a URI, you can either identify or block the entity from being synced.

Please, consider that, the error you are getting is based on the idea that having a bundle synced, all entities should have a URI. That is why if a bundle is mapped, the getUri should always return a value and we do not allow it to return null. That is because a URI is like a persistent identifier in RDF data and thus we thought we should be strict with it.

I checked in my Virtuoso, it seems all the 1965 entities had been synchonized. Why still the error occured?

This might be that the URI was removed somewhere along the way or that Virtuoso already had the data before you sync. If you are using docker, and the container crashed, it doesn't mean data were deleted if the volume remained intact. I mean, we would need more information to identify further issues.

In terms of this ticket, I will review the second MR because in the first one, the deletion of triples was removed and now data cannot be removed when the entity is deleted.

We discussed with the maintainer, a new subscribed event will be thrown before the syncing occurs so that entities to be indexed can be altered.

idimopoulos → created an issue.

Let's focus in a single ticket. This is an active bug-ish thing. Closing this as a duplicate of https://www.drupal.org/project/drupal/issues/3050261 🐛 index.php randomly appears in friendly URLs Active and adding the relevant link for anyone that is interested in (google pointed me to this link first).

Note that according to https://www.drupal.org/project/drupal/issues/3050261#comment-13118805 🐛 index.php randomly appears in friendly URLs Active , there is a working workaround to use redirect module's forced clean URLs to automatically remove the prefix on request.

I am not adding anything to the argument, the patch has solved the problem for me too where I was setting the value of the product to 300 VAT included, and in the checkout, I was getting 300.01. Very bad experience for the client too. I am adding the patch that applies to 2.x.

NOTE: Some of the fragments of the patch in the BuyXGetY class were not there anymore. Code was removed. I discarded them. Take care whoever uses it. It might be that more is needed there.

The \Drupal\Tests\email_contact\Functional\EmailContactLinkFormatterTest::testFormatter should already suffice for the testing part.

idimopoulos → created an issue.

Thanks. Looks good.

idimopoulos → created an issue.

claudiu.cristea → credited idimopoulos → .

idimopoulos → changed the visibility of the branch 2381293-delete-action-10.2.x to hidden.

I think this is back to active since https://www.drupal.org/project/state_machine/issues/3058874 ✨ Allow same From/To state Fixed has been merged. Now that same from/to state transitions are allowed, they should adhere to proper check from the guard class.

This should include an update that goes through the roles and updates the permissions.

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

For reference, this is reproducible only if the user does not have the access to delete the entity removed. In web/modules/contrib/inline_entity_form/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php:914 there is a check already

    if (!empty($entity_id) && $this->getSetting('removed_reference') === self::REMOVED_OPTIONAL && $entity->access('delete')) {

and the delete checkbox is not added if the user does not have 'delete' access. This, checking either for the access or if the array key exists is a valid case.
I don't know if tests are needed as this is a warning and a simple missing check that does not affect the end result. The user does not have access, the entity is also not marked for deletion.

These are just failsafe's so I didn't provide tests. I don't think they are necessary.

idimopoulos → created an issue.

Fixed a php sniff.

I am making a slight change to check for the ID, not just the entity. The reason is that OG menu seems to break the layout builder which passes as context an entity without an ID.

idimopoulos → created an issue.

A but more precise with this:
I am defining 3 paragraphs. 1 for a single layout, 1 for the accordion, 1 for the accordion item.
Then I have in the entity a paragraphs revision for the paragraphs which is using layout builder.
Then I have the 1 column layout use the stable paragraphs.
And that is how it is indeed the problem is reproduced. The patch fixes the issue. +1 RTBC. Tested in vanilla Drupal

I prefer no to see any changes in a module configuration if it works like it did before

It is a fair explanation for me. I will leave it to the maintainer. I would still prefer the other way, but I don't have a reason to insist :) +1 RTBC

This is working as described but I would like to challenge the BC approach. In the MR, you are optionally saving the value in the config yaml file, in order to maintain BC. You are also providing a test that saving the settings will omit the value if it is the default (to test BC).
Why not simply provide an update path which update the config file to include the default value as a standard way of storing it? that way, you do not even need tests to verify the changes, and the projects adapting to it, can use tests that cover hook updates. I don't think that BC means you cannot change stuff. It only means that existing projects should work as they already do.
If there is a reason for that, then it is RTBC from me (apart from one typo nitpick).

We tried this in our project, fair request, good improvement.

idimopoulos → created an issue.

Good simple addition.

This turned out to not be really something that is worth intervening here. Core logs an exception by default and should not be part of this module to force avoid that.
Projects should either handle this explicitly or it should be handled in core. Closing.

idimopoulos → created an issue.

So, the MR does:
* Prevent saving and loading entities when their subject contains unsupported characters.
* Prevent attempting to load entities in the router parameter converter class, when the passed URI contains unsupported characters.

idimopoulos → created an issue.

@elber may I ask what you are trying to achieve? SPARQL is supposed to work with string IDs and more specifically, adhere to the RDF standard that the entity ID is a URI mainly.
Now, that does not mean that a resource URI cannot be a number, for example "<44>", however, it might be preferable to adapt the entity IDs to be string before they are passed in the method here. Further more, the code you provided, will not construct the <resource> resource URI (encapsulated with <>). Does this even work? I don't think it makes a valid RDF scheme to have data with simple numeric ID.
If you still need them to be numeric ID though, I think you should use some hook_entity_load in order to typecast the ID.

idimopoulos → created an issue.

idimopoulos → created an issue.

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

Straight fix.

Thanks for the work :)

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

Rerolled patch 66 for 10.1.x and 11.x.
However, I should note that this is a blind reroll. Did not check if there are any changes in core that might affect this.

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

idimopoulos → created an issue.

I have opened an MR in gitlab only so that anyone that needs it now and cannot wait for a merge here, can follow https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → in order to update. I used the patch from #7

+1RTBC here :)

Adding https://www.drupal.org/project/entity_legal/issues/3358135 🐛 Missing query access check WSOD Closed: duplicate as a related issue only because that issue is more specific to something that was already fixed in the current one without being in the scope.

Patch applies and works for us. Also, comments from #13 are addressed. Tests are green and applying.
+1 RTBC

Tested, works, +1 RTBC. The fix is pretty straightforward and a good improvement though I am not sure how much value does it since I doubt many are creating entities with string ID. Still, works for us.

As agreed, we will ignore 7.4. It will be removed. RTBC+1.

I added 1 more commit because there are cases for the inline editors that breaks the code.
This is for example when there are multiple text filter plugins, for example \Drupal\filter\Plugin\Filter\FilterAlign::process which adds the class using the dom element, thus adding it as a string.

That causes

        $variables['image']['#attributes']['class'][] = 'no-image-style';

to throw an exception. I am handling this case by splitting the class apart first.

Adding https://www.drupal.org/project/svg_image/issues/3321542 → as a related issue which was about the same thing mainly.

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

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

idimopoulos → created an issue.

idimopoulos → created an issue.

Straight reroll.

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

We should decide which patch to use. It is preferable though to go with Gitlab.
I added a small fix that covers a false negative due to a wrong type check.

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

Ugh... True... sorry, bad miss on my side. Added the message to the interface.

@webflo, the deprecation message has already been added. Please, check the Gitlab MR rather than the attached file. Hiding attached file.

Thanks for the work.