🇩🇪Germany @mxh

Thanks for updating the merge request.

Setting to NW because the tests are failing: https://git.drupalcode.org/issue/rules-2816033/-/jobs/3962405#L1017
The same error is happening often times in the tests. The test failure is actually happening since the merge request was created, so it's not because of the rebase that just happened.

It turns out the merge request that was initially created in #160 does not 100% reflect the patch of #156. It is different, seems the contents were manually copied over and some parts were forgotten or copy-pasted wrong.

What I can see for example is that this line https://git.drupalcode.org/project/rules/-/merge_requests/21/diffs?commi...
still includes the event_subscriber tag definition whereas within the patch of #156 this definition was removed. And this is most probably the reason why the tests are currently failing in the merge request, as the main point of the patch is to let the listeners being subscribed by LazyGenericEventSubscriber.

We first need to make sure it 100% reflects those changes from #156, then rebase (and then make further changes if needed).

Appreciating the attempt to update the MR. However, as long a the MR is not mergeable and does not properly show the relevant code parts that are requested to get merged, this cannot be reviewed. Thus setting to NW.

When providing and updated version of the patch, please also provide an interdiff file from the last one that got mostly perceived as working, which was the one from #156. In that comment you can also find an example of such an interdiff file. It's an important piece of the patching workflow in order to efficiently notice the differences for anybody. I know that the patch is most probably only provided for patching the codebase and not as part of the review process here, but still it should be included.

Re-rolled against the latest release (1.0-beta12)

Merge request and patch need a reroll for the latest release.

When using basic_auth module, user authentication is happening on Drupal-level using HTTP basic authentication. So why would you want to login when you're already authenticated? It doesn't make sense.

Looking at the IS and since you're the only one who reported such a problem, the cause of the problem is/was elsewhere and thus it looks this issue got outdated.

This sound like a special requirement that needs the combination of some modules, such as ECA → and maybe ECA VBO → . But this module is not covering anything of your requirement and it's not supposed to do so. I guess this is something that you could have find out by yourself in the meantime and you could have reported your findings back here.

We are building Model where the PreSave Event of a GroupRelationship is relevant

This sounds like an ECA-specific requirement, which is not within the scope of this module.

mxh → created an issue.

Experiencing the same that the database table still contains group relationship items while the according group does not exist anymore.

We're having lots of concurrent requests going on, and one thing I could imagine is that the deletion process may take some time, especially when having many group relationship items. And within that time frame another request might have just added a new relationship to the group that is about to be deleted. But this is just a first guess on finding out what could have gone wrong.

mxh → created an issue.

There are still comments made from @avpaderno 10 months ago that have not yet been properly resolved. Example: https://git.drupalcode.org/project/parameters/-/merge_requests/5/diffs#6...

mxh → created an issue.

Patch attached that is the same as the current merge request.

Created a merge request containing a possible fix. It works for me locally but more testing is necessary.

Unfortunately this is certainly not fixed yet. See comment: https://www.drupal.org/project/views_entity_form_field/issues/3462770#co... 🐛 Field values get swapped among different entities in a view Active

Turns out this issue is not yet fixed with the linked issue from #4. This is actually a bug cuased by a different place in code.

Problem is located at Drupal\views_entity_form_field\Plugin\views\field\EntityFormField::buildEntities:

<?php
  protected function buildEntities(array &$form, FormStateInterface $form_state, $validate = FALSE) {
    $field_name = $this->definition['field_name'];

    // Set this value back to it's relevant entity from each row.
    foreach ($this->getView()->result as $row_index => $row) {
      // Check to make sure that this entity has a relevant field.
      $entity = $this->getEntity($row);
      if ($entity && $entity->hasField($field_name) && $this->getBundleFieldDefinition($entity->bundle())->isDisplayConfigurable('form')) {
        // Get current entity field values.
        $items = $entity->get($field_name)->filterEmptyItems();

        // Extract values.
        $this->getPluginInstance($entity->bundle())->extractFormValues($items, $form[$this->options['id']][$row_index], $form_state);

        // Validate entity and add violations to field widget.
        if ($validate) {
          $violations = $items->validate();
          if ($violations->count() > 0) {
            $this->getPluginInstance($entity->bundle())->flagErrors($items, $violations, $form[$this->options['id']][$row_index], $form_state);
          }
        }
      }
    }
  }
?>

Here the mapping for building the entity from submitted values is still done by the views's row index ($row_index variable). It can be related to a different entity when a new one got added in the meantime.

Additional note to reproduce this problem: It particularly appears when using an Ajax field, such as a file upload field:

1. Open a view using a file upload field as Views Entity Form Field. Upload a new file or remove an existing one. Don't click on save yet.
2. Open another tab, create new entity of same type and save it
3. Switch to the first tab (no refreshed page) and click on the save button of the Views form.

Great news, thank you!

Module should work fine with D11.

mxh → created an issue.

Another thought I had when looking at this: When any (imported/extracted/migrated?) icon is a plugin definition, then there may potentially be a lot of plugin definitions to be collected. From what I've seen on most sites, only a (very) small subset of icons of a library would be displayed on a website.

One example: many sites include Bootstrap icons or Google material icons as a whole library, but in the end they just use 10-20 of them in total, wheras the library itself comes with hundreds. There are other icon libraries around that may have thousands. If we're loading a plugin, then a default plugin manager will first load all existing plugin definitions (mostly from a cache backend) into memory and will keep them until the end of its service lifecycle. When only a small amount of the definitions are finally being used, this may be worth to be addressed for optimization.

"Extractors" remind me of plugin derivers. "Icon pack" reminds me of a base plugin ID. For me icons have characteristics of a (single-directory) component. Just a bit concerned that it might be over-complicated DX, especially when looking atalready existing core concepts. In the end it's just about icons.

I briefly tested with '#tags' enabled, however multiple groups are not yet working. It is currently only adding to one group.

I'm going to merge this one as it works on my testing and already gives some UX improvements. Support for multiple groups may be addressed by a separate issue.

Thank you @nielsonm for kicking this off and helping out.

mxh → changed the visibility of the branch 3481519-add-autocomplete-to to active.

False alarm, this is caused by some other code.

Investigating a bit more.

mxh → created an issue.

Resolved #6, setting back to NR.

Unfortunately this breaks the existing functionality for entering ID / UUID only.

Thanks for creating the issue and the merge request. Is it ready for "Needs review"?

Same slow query is happening within group_entity_access function so that part may be optimized as well using the cache.

As per description of the Group Action's module page:

This module adds configurable actions regarding Group functionality, such as adding a user as member, adding or removing content.

I see none of that is part of your requirements described in the IS, so I don't understand why the issue was posted here. But I see a lot of ECA components involved, so this looks like an issue for ECA.

Edit: just seen that you already created basically the same issue in ECA: 💬 Working with groups module, check if user is a member of the group and if they are the admin of that group. Active Please avoid creating such duplicates. Therefore closing as duplicate.

You obviously don't know what you are doing. What I can see is that you are removing whole code blocks without providing any replacement. It means if someone would actually commit your garbage, it would break existing functionality.

Example: https://git.drupalcode.org/project/parameters/-/merge_requests/6/diffs#d...

This is grossly negligent and I'm considering to report you for the attempt of breaking things. Because I don't know whether that was on purpose or you are just blindly applying some CLI tools without thinking of what you are actually doing. At least you won't get any credit for this from me.

mxh → created an issue.

I think the originating problem of this issue is covered in 🐛 Fields are incorrectly saved if new entites are added Fixed
That issue is fixed but there was no new release created so you might still encounter the problem. You could try with the latest dev version and look whether the problem still persists.

mxh → created an issue.

Great :) I guess the module is missing a bit of good UX. But there should be ways to make it easier.

I'm also open for improving Group Actions → module in case you think there's something missing. For example, the autocomplete input is not in there, but could be added. Feel free to open an issue and ideally with a merge request there.

Committed, pushing new release for this fix.

Thanks for the report and provided MR, looks good.

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

Reason?

mxh → created an issue.

Queries being built up using query access are exploding by their statement length using hundreds (sometimes thousands) of placeholders.

This case is true when the number of groups grow accordingly. Usually a user has one membership per group anyway.

Another problem is the following query being built within \Drupal\group\QueryAccess\EntityQueryAlter::doAlter:

<?php
// ...
$group_relationship_data_table = $this->entityTypeManager->getDefinition('group_relationship')->getDataTable();
$plugin_ids_in_use = $this->database
  ->select($group_relationship_data_table, 'gc')
  ->fields('gc', ['plugin_id'])
  ->condition('plugin_id', $plugin_ids, 'IN')
  ->distinct()
  ->execute()
  ->fetchCol();
// ...
?>

Since the table may grow by hundreds of thousands of records, the distinct query affects performance. It gets noticeable when this method is called within one record up to 100 times (which is possible when rendering Views configurations).

@mxhcache tag lookups can be replaced with redis so they won't hit the database at all.

Thanks for the reply and tip @catch using Redis might actually an option to try out. My key point in #9 is that it doesn't make sense to replace a Porsche with a Ferrari for getting faster when the speed limit is at 50 on a one-lane bridge. I'm sorry that I might have brought in something off-top here - will move it into a separate issue in case there's more room for discussion on it.

For remaining tasks:
Maybe we then also need an alternative mechanic for media_library since it's build on top of a view configuration. Maybe any Drupal core module that ships with something on top of a Views config?

The main problem for why Drupal is slow is, because of getting entity data out of the database is slow.

Is that statement backed by any performance numbers / benchmarks?

What I've often seen as a performance bottleneck is not the database itself, but the connection to it. When a Drupal site grows (showing in its number of configuration items), one request to a Drupal page may take up hundreds, sometimes over one thousand of database queries. This is because of cache tag lookups, config reads and finally when content entities are queried. When having a local database without any latency, performance is usually not a problem.

Wondering if this is possible with ECA VBO ? (Ask for user input)

Yes, it's possible using the plugins (events and actions) provided by this module.

mxh → created an issue.

mxh → created an issue.

Over one year without any response, not really helpful for anyone.

The idea is to manage it on the level of fields (with their available widgets). Similar like you can use Layout Builder instead of the Manage Display configuration page, i.e. the configuration page of Manage Form Display would be replaced by Experience Builder for arranging the fields and configuring their widgets.

I mentioned the example of the Field Group module. The site builder may instead create such field groups within the Experience Builder, and placing configured field widgets into such a group.

Such thing immediately brings up the question of "why re-inventing the wheel" but I think it's a necessary thought when we want a more consistent UX for the site builder.

I think point 3 in the IS relates to the linked issue. When that could be confirmed by either one of you, I may be able to get resources for addressing this and work on an optimization approach (regarding point 3). Since we're dealing with content entity data, I'd prefer a way to directly filter on database level and not on application level. However this may seem like a whole architectural change since this is currently part of the flexible permissions being calculated.

mxh → created an issue.

mxh → created an issue.

Tests are looking good, thank you a lot for adding them! +1 for RTBC.

With the validation handler, I could imagine that we implement an ECA condition which takes a content entity as an argument and runs it through its validation handler. It returns TRUE or FALSE, depending on the result of the handler.

Taken from #5, I think it really makes sense to also have such a condition plugin available. It may be especially useful for example when creating new entities on the fly coming from external APIs. We could address it in a separate issue.

I'm 100% sure the ID collision is real and it needs to get properly fixed.
The reasons why I'm sure about this are already written in my previous comments.

The added test proves ID collision in the following way:

• The test fails in the current branch as MR444 shows.
• The test added into MR444 (https://git.drupalcode.org/project/eca/-/merge_requests/444/diffs) also fails when running on tag 2.0.1.
• The test is green on MR 443 which confirms that by adding more entropy into the ID, the collision is not happening anymore./li>

The test itself contains the same problematic configuration as provided in the IS: sharing same cron event IDs.
The test failures are not coming from the static variable or because cron is run multiple times per runtime.
The test fails because of the ID collision.

More entropy has been added in MR443 and that is why the test is green.
I won't rewrite the test because I'm convinced it's proving my arguments properly, like other core tests also run cron multiple times per runtime.

I'm a bit frustrated that I don't get any support here. When not properly fixed, this will mean the cron events won't trigger reliable anymore.
At least I know what to do then (which means patching), but I'd like to have this problem be avoided for others. For that I've spent a lot of time already, explaining, giving examples and finally providing a possible fix including a test.

mxh → created an issue.

This module is already compatible with Drupal 11.

The problem with this test architecture is that the Drupal cron is run multiple times within a single PHP request.

I'm sorry but I currently disagree with this argument. Testing against multiple cron runs within one runtime is valid. For example, Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTest::testDeletion also runs cron multiple times within one test. There are various other tests in Drupal Core that run cron multiple times.

You're now assuming that cron runs only one time per request / runtime. But there is no assertion which guarantees that. There is even a test in core that tests against running within the same request: Drupal\Tests\system\Kernel\System\CronQueueTest::testLeaseTime

Fixes included in related issue.

mxh → created an issue.

Unfortunately there is a bug in VBO that breaks Views UI when a preconfigure form is being implemented. Adding related issue.

Wrong issue.

Unfortunately there is a bug in VBO that may break the Views UI. Relating to that bug report.

I have set this to Major because it breaks the Views UI. Feel free to set a different priority if wrong.

mxh → created an issue.

The section "Steps to reproduce" does not contain a distinct way for me to be able to reproduce this.
Besides providing distinct steps to reproduce, please also provide a simple configuration export (only relevant config files) that contain your VBO config that may help to see what the problem is you're encountering.

Did you read the README and carefully followed its instructions?

What version of Drupal core and Views Bulk Operations are you using?

There are currently more than 150 reported site installations. Either they all have non-working VBO configs or you did not follow along the README instructions or you might use a modified / outdated version of VBO.

Readme got adjustified accordingly. For any site making use of the edge-case, please have a look at the new README contents.

mxh → created an issue.

mxh → created an issue.

Added a solution that works as follows: when there is no configuration form item added, i.e. the form itself stays empty, then a redirect to the confirm form will be performed.