🇳🇴Norway @efpapado

efpapado → changed the visibility of the branch 3518109-the-new-revision to hidden.

MR created.

efpapado → created an issue.

efpapado → created an issue.

I updated the hooks to also include the view ID and the display ID.

efpapado → created an issue. See original summary → .

Following up vegardjo's suggestion, another code snippet that does not trigger a node save:

    $connection = \Drupal::database();
    $bundles = [
      'my_node_bundle',
      'my_other_node_bundle',
    ];
    $nodes = \Drupal::entityQuery('node')
      ->condition('type', $bundles, 'IN')
      ->accessCheck(FALSE)
      ->execute();
    foreach ($nodes as $nid) {
      $connection->merge('key_value')
        ->keys([
          'collection' => "pathauto_state.node",
          'name' => $nid,
        ])
        ->fields([
          'value' => serialize(1),
        ])
        ->execute();
    }

This only updates the key_value table, which immitates the class PathautoState 's logic.

And here too: use at own risk!

New merge request against the newest version of the module.

Set to Needs Review.

efpapado → created an issue.

Adding https://www.drupal.org/project/openai_batch → under "Have not reviewed yet" category.

@graber Thanks for your quick reply. I started digging the code, here is what I found:

Both modules use the core class \Drupal\views\Form\ViewsForm that adds the form functionality to the view.
For both modules, when the form is submitted this function is called: \Drupal\views\Form\ViewsForm::submitForm which then calls the \Drupal\views\Form\ViewsFormMainForm::submitForm, which then does this:

foreach ($view->field as $field) {
  if (method_exists($field, 'viewsFormSubmit')) {
    $field->viewsFormSubmit($form, $form_state);
  }
}

Similar logic is followed on the validation step, in \Drupal\views\Form\ViewsFormMainForm::validateForm

By looking at this, I understand that the core's logic is that all the possible fields that contain a form are supposed to have their forms validated/submitted, and then each module's validation/submit handler is responsible to figure out whether it should do something or not. So I think that what you wrote:

I see Views adds a "Save" button by default.. I think every contrib module should unset that and set one of its own.. and core Views shouldn't provide that default button at all in the first place.

is basically correct as approach.

I experimented a bit with having VEFF adding its own button, and calling its own submit handlers independently. It worked more or less, but new problems arose. When a VEFF field is added in the form, the VBO's validation breaks:

  public function viewsFormValidate(&$form, FormStateInterface $form_state) {
    if ($this->options['buttons']) {
      $trigger = $form_state->getTriggeringElement();
      $action_delta = \end($trigger['#parents']);
      $form_state->setValue('action', $action_delta);
    }
    else {
      $action_delta = $form_state->getValue('action');
    }

In case of VBO's non-buttons configuration, the last line $action_delta = $form_state->getValue('action'); expects a flat values array.
But claro is moving the VBO stuff in another container:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/claro/...

function claro_form_alter(array &$form, FormStateInterface $form_state, $form_id) {
...
    // Determine if the Views form includes a bulk operations form. If it does,
    // move it to the bottom and remove the second bulk operations submit.
    foreach (Element::children($form['header']) as $key) {
      if (str_contains($key, '_bulk_form')) {
        // Move the bulk actions form from the header to its own container.
        $form['bulk_actions_container'] = $form['header'][$key];
        unset($form['header'][$key]);

        // Remove the supplementary bulk operations submit button as it appears
        // in the same location the form was moved to.
        unset($form['actions']);
...

and because VEFF is adding to the form a #tree => TRUE, the $action_delta = $form_state->getValue('action'); returns NULL.
This last part must become $action_delta = $form_state->getValue(['bulk_actions_container', 'action']); in order to return the correct delta.

Even when bypassing this, because of the core class's behaviour described above, whatever button triggers the submission, all the submit handlers will be called. So if the extra button offered by VEFF is clicked, VBO's submit handler will also be called (and also the opposite).

I suggest we add on both viewsFormValidate and viewsFormSubmit

$trigger = $form_state->getTriggeringElement();
// some logic that will return, if the button that submitted the form was not the VBO one

This of course must be done also on VEFF. And possibly we need to document it on the core's class.

Also, we need to make VBO not change its behaviour based on the theme's or other modules' changes. Thus find a more concrete way to understand the context of the form submission (#tree or not, own container or not).

Before proceeding with the above, I'd like to ask your opinion based on your insight.

efpapado → created an issue.

Hello, not sure what you mean. The blurhash is just a string, if you enable the blurhash_image_style_metadata submodule, when the metadata are generated, the blurhash string is stored in the database (on the entity's table) and it gets exposed in JSON:API as the other fields.

Or maybe you mean something else that I didn't get? :)

efpapado → created an issue.

I merged the PR :)

MR created.

efpapado → changed the visibility of the branch 8.x-1.x to hidden.

efpapado → created an issue.

I suggest you merge it, and we can keep improving it :)
There will be other suggestions also for improvements.

Merge request created.

efpapado → created an issue.

Created merge request.

efpapado → changed the visibility of the branch 3472296-provide-an-option to hidden.

efpapado → created an issue.

Setting it back to needs work, the tests are failing, although they succeed locally. I will investigate further...

Removed a useless assertion that was breaking the tests.

I created another module that does this. It stores the metadata in content entities. I have also included a submodule that copies this module's logic: https://www.drupal.org/project/image_style_metadata →
We could discuss about deduping the effort and see how the 2 modules can cooperate :)

Is it ok now?

Added type hints to api, added a test. Not sure about the issue summary update, isn't the change record enough?

After some thought, I added another argument in the hook, which is the id of the image style. Although by default it can be derived from the derivative image uri, there can be cases where this is not possible (for example a custom implementation of an external file system like s3fs.)

Updated also the change record.

Updated the patch for the 11.x branch, and created a draft change record.

efpapado → created an issue.

thejimbirch → credited efpapado → .

efpapado → created an issue.

Here is also a patch (the exact same diff) for 10.3.x

I think that the problem is on \Drupal\options\Plugin\Field\FieldType\ListItemBase::validateAllowedValues:

      if ($current_element['item']['key']['#value'] !== NULL && $current_element['item']['label']['#value']) {
        return $current_element['item']['key']['#value'] . '|' . $current_element['item']['label']['#value'];
      }
      elseif ($current_element['item']['key']['#value']) {
        return $current_element['item']['key']['#value'];
      }
      elseif ($current_element['item']['label']['#value']) {
        return $current_element['item']['label']['#value'];
      }

      return NULL;

The first condition casts the label value to boolean, so the 0 (string or integer) gets rejected, and then the following 2 do the same for the key and value again, so NULL is returned.

I gave it a try here: https://git.drupalcode.org/project/drupal/-/merge_requests/9096/diffs

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

efpapado → created an issue.

Pushed new commit with a comment as requested.

Hiding the patch file, as a MR has now been used, and changing the version to 11.x

Reopening the issue since the problem is still here on D10.2.3.

This is the relevant part of the stacktrace:

EntityChangesDetectionTrait.php:34, Drupal\Core\Entity\ContentEntityBase->traitGetFieldsToSkipFromTranslationChangesCheck()
ContentEntityBase.php:1434, Drupal\Core\Entity\ContentEntityBase->getFieldsToSkipFromTranslationChangesCheck()
ContentEntityBase.php:1475, Drupal\Core\Entity\ContentEntityBase->hasTranslationChanges()
ChangedItem.php:49, Drupal\Core\Field\Plugin\Field\FieldType\ChangedItem->preSave()
FieldItemList.php:233, Drupal\Core\Field\FieldItemList->delegateMethod()
FieldItemList.php:191, Drupal\Core\Field\FieldItemList->preSave()
ContentEntityStorageBase.php:938, Drupal\Core\Entity\ContentEntityStorageBase->invokeFieldMethod()
ContentEntityStorageBase.php:888, Drupal\Core\Entity\ContentEntityStorageBase->invokeHook()
EntityStorageBase.php:529, Drupal\Core\Entity\EntityStorageBase->doPreSave()
ContentEntityStorageBase.php:753, Drupal\Core\Entity\ContentEntityStorageBase->doPreSave()
EntityStorageBase.php:483, Drupal\Core\Entity\EntityStorageBase->save()
SqlContentEntityStorage.php:806, Drupal\Core\Entity\Sql\SqlContentEntityStorage->save()
EntityBase.php:352, Drupal\Core\Entity\EntityBase->save()
...

@larowlan

Looking at \Drupal\Core\Entity\ContentEntityStorageBase::populateAffectedRevisionTranslations which I think would be the calling parent here, can you check if it gets inside the if statement

    if ($this->entityType->isTranslatable() && $this->entityType->isRevisionable()) {

I suspect it should not, so you might need to check those two methods.

Up until the array_flip that throws the warning, the \Drupal\Core\Entity\ContentEntityStorageBase::populateAffectedRevisionTranslations is not called at all.

I created a merge request.

Created a merge request, please review.

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

I confirm that the patch works.

Rephrased title.

efpapado → created an issue.

I'm uploading for now a newer version of the patch to apply on 2.2.5 and I can check your replies about a different process.

New patch for 11.x

efpapado → created an issue.

efpapado → created an issue.

efpapado → created an issue.

Proposed patch.

efpapado → created an issue.

Fixed.

efpapado → created an issue.

Setting to Needs review.

Thank you for posting your solution, I had exactly the same problem and took me quite a while debugging :)

Ping to the maintainers.

New patch that extends the rektor one.

Another try.

Done.

Thank you, patch committed, new release coming soon.

New patch uploaded, based on bot and upgrade_status module recommendations.

New patch uploaded, based on the patches above and the suggestions from the upgrade_status module.

Fixed PHPCS problems.

I'm proposing another patch, which has (almost) the same code as yours, but on a different class: Instead of invoking the hook on ImageStyleDownloadController::deliver(), I invoke it on ImageStyle::createDerivative() to make sure that the hook is invoked also when the derivative is created by other methods (for example through the image_style_warmer module).

The patch does apply on 10.1.x. Setting to Needs review and retesting.

Proposing patch.

efpapado → created an issue.

I'll argue a bit more, reminding that even I myself have not yet concluded on what should be the desired behaviour.

The way it is implemented now has an internal inconsistency to the plugin itself. The plugin can be configured to run with:

 * - method: (optional) What to do if the input value is empty. Possible values:
 *   - row: Skips the entire row when an empty value is encountered.
 *   - process: Prevents further processing of the input property when the value
 *     is empty.

The "row" method skips the target entity completely, no entity is being created. But the "process" method does not actually skip the field, it does update it with null value, which is equivalent to the source actually having something null-ish.
If the "process" behaviour is what we want, then shouldn't the "row" method also save a new entity with all the values null/default?

I understand the "skip" intention to be "don't touch it":
Row on new entity: Don't touch it, so don't save a new one.
Row on existing entity: Don't touch it, so don't update it at all.
Process on new entity: Don't touch it, so let it get it's default value as it has been declared in the field configuration.
Process on existing entity: Don't touch it, so let it keep what it already has, if it has something.

Here's a first approach.

efpapado → created an issue.