🇺🇦Ukraine @HitchShock

+1 Nice feature for a custom entities

Hi all.
First of all, I want to thank everyone who is working on this task. It solves the performance problem for big data entities in certain cases.

But I also found a possible way to make it better for some scenarios.
We can remove `$type === 'INNER'` from the condition.
If the table is the same, then it doesn't matter which type of join is used. Anyway, the same table will be used.
Removing this condition can be useful for big data queries with base fields of the entity, which are stored in the same table if the data table is the same as the base table for an entity.

For example,
- we have a `custom_entity`
- we send a simple entity query to get IDs sorted by uuid
- the default query will be

SELECT base_table.id AS id, base_table.id AS base_table_id, custom_entity.uuid AS uuid
FROM
custom_entity base_table
LEFT JOIN custom_entity custom_entity ON custom_entity.id = base_table.id
ORDER BY custom_entity.uuid ASC
LIMIT 20 OFFSET 0

What is the problem? If custom_entity is a big data entity, then we are trying to join big data to big data, which will take much more time than without 'join'. This impacts performance a lot

If we remove `$type === 'INNER'` part of the condition, the issue will be solved, because the query will be generated like

SELECT base_table.id AS id, base_table.id AS base_table_id, base_table.uuid AS uuid
FROM
custom_entity base_table
ORDER BY base_table.uuid ASC
LIMIT 20 OFFSET 0

I added a hidden patch with this fix.

P.S. Please let me know if my opinion is correct or if it has obvious flaws in the context of Drupal core

Hi @oily
I answered your comments in the PR. You can check them

P.S. I have decided to give this issue a second chance :)

@acbramley

We need to move these new methods somewhere else

It doesn't make sense to add new methods to an interface/class that we know is being deprecated. We can postpone this issue on that one instead if you like.

Where exactly to move?
My opinion is that new methods must be in the same place where revisionIds() is located, which is logical, but I'd be happy to hear another suggestion.
And I'm not sure it's right to postpone a critical issue because of a minor one.

That code is ancient and is part of the reason why that class is being deprecated, it's not compatible with all db engines. We should do it properly first since we're introducing new functionality, not leave it to a follow up to refactor again.

But it's not a deprecated class yet, we just have a ticket to make it so, and it's a big question when that ticket will be resolved. And which of the tickets will be solved first.
If this one is merged first, then that one will be refactored according to 11.x. If that one will be merged first, then this one will be refactored.
For example, we had to refactor this one after merging the 📌 OOP hooks using event dispatcher Needs review

So I see the following ways:
1. Keep it as it is, cuz the other ("normal" priority) refactoring ticket cannot block the "critical" one, this issue breaks the content on the site and must be fixed asap. Later in the context of #3396062 the code will be refactored
2. To locate new methods in the same place as a revisionIds() method, we can make a separate branch as a combination of #2977362 and #3396062 + keep the current one. The two branches should always be up to date. But I don't think this is the right way to combine the two tickets, because it could delay the resolving of a critical issue, but this is a possible way.

I would like to hear other opinions. Maybe even in the form of a vote :)

Approve that it is still reproducible for drush en <module>

Drupal: 10.4.6
Drush: 12.5.3

Hi @acbramley
I understand your point, but:
- That ticket is still in progress, so it doesn't make sense to move the functions elsewhere for now, because it's still a big question which of the tasks will be solved first.
Since there will be a merge conflict here, the task that will merge last will fix it. So, we can ignore it for now.
- A similar opinion as above, since all class methods currently use db queries, it should be the same here. Their refactoring will be part of #3396062 or will happen here if that task is merged first.
Especially since we can't use entity queries to get these values. It will at least be select queries

Conclusion: Your comment cannot be a blocker for this ticket.

P.S. There is also a related bug described here 🐛 Skip changing 'revision translation affected' field if the old revision is syncing Needs work . Without that patch, a revision_translation_affected property might be changed, which can cause the issue when we see a revision of the other(unexpected) language in the revisions list.

Steps to reproduce:

• User A - created a node EN - revision 1
• User B - created a translation ES - revision 2
• User A - updated a translation EN - revsion 3
• Revision list:
  • EN - revisions 1 and 3
  • ES - revision 2
• Remove User B
• Revision list:
  • EN - revisions 1, 2, 3
  • ES - revision 2

Reroled and refactored the fix.
Updated tests.
Also, provided patch files for different versions,

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

+1
Files of the library have been updated several times, but the version hasn't been updated for a long time.
So, please, remove the library version or don't forget to update it every time the library files are updated.

I like the solution to remove the version.
So, RTBC.

+1 Caught the same issue
The problem is that I can't find any default features that will allow us to reproduce it with a simple core settings.
But if we will add a custom constraint to the field with Media library, then it won't be working, because of the reason mentioned in the description.

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

hitchshock → created an issue.

hitchshock → created an issue.

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

The last patch works for me and also fixes the critical conflict with entity_reference_revisions module.
+1 RTBC

@socialnicheguru functionality conflicts or code conflicts inside the *group.module*?
If it's just a code conflicts, then no extra moves are needed here since there are no conflicts with the dev branches

hitchshock → created an issue.

MR must be rebased from the dev branch

A fix will be provided in the next release

Thanks all for the help.
I reverted some changes to keep the module compatible with other packages and older versions of Drupal.
For example:
- I reverted "spatie/pdf-to-image": "*" cuz our module doesn't support version >=3.0
- We still need to use FileSystemInterface for older Drupal versions

A fix will be provided in the next release

@carolpettirossi Nice catch! Thanks!

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

Provided CI and fixed phpstan/phpcs/cspell errors.

hitchshock → created an issue.

Done!

@candelas Thanks for the issue.
I will use this ticket to clean up the outdated generator commands.
Updated the title and description.

@chrisscrumping Thanks for the patch.
Even though the bug was fixed in the context of 📌 Automated Drupal 11 compatibility fixes for migrate_file_to_media Needs review I would prefer to give you credit for your activities

Updated project requirements.
The minimal Drupal version is 10.3 from now.
Fixed phpstan errors on the project to make it compatible with D10/D11

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

Thanks @carolpettirossi for the patch.
I made a small change in the MR.
An update will be available in the next release.

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

hitchshock → created an issue.

hitchshock → created an issue.

Fixed caching

Implemented bulk operation

hitchshock → created an issue.

Implemented a new config

hitchshock → created an issue.

Installed CI

Added filters to the list page

hitchshock → created an issue.

hitchshock → created an issue.

hitchshock → created an issue.

hitchshock → created an issue.

hitchshock → created an issue.

Refactored the code to make it D11-compatible

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

Implemented a patch + added new fixes.

About the issue from bjorn, it was caused by an error during installation. That error prevented the creation of the entity table.

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

@lazzyvn As you wish, but this is a form for testing. I don't believe that someone uses it regularly and for many users. So, I don't understand how it changes the module design.
The form still works as expected, we can still send the message to everyone or selected users (separated by a comma). This is more than enough for testing.

hitchshock → created an issue.

HitchShock → created an issue.

The module social_swiftmail was already migrated to symfony mailer but swiftmail module still exists in composer.json requirements.
It must be removed from there ASAP, it breaks the composer audit and prevents a normal update process

I see that the simpler the work, the more it is scrutinized, and everyone sees something different and changes the same thing several times. And they pick on such trifles that are absolutely not critical and will not affect the overall functionality.
Instead of quickly implementing a simple change, everyone is engaged in bureaucratic ping-pong for no real reason.

In a month, someone else will come and say that they don't like some error text (which is generally displayed only when UI validation is disabled, which is almost never) or comment text.

I'm tired of fulfilling the whims of everyone who comes to check the task. So I'm stopping working on the issue.

Also added a patch

HitchShock → created an issue.

Solved the last PR issues

A solution #6 is pretty good for me.
I just updated it a bit:
- it's better to use regex patterns to search and replace values
- sometimes we need also to update config keys (not only values)
- it's not required to update IDs of the Relationship Types. We can keep them to prevent possible errors with IDs mentioned in #15

And of course, you need to update your custom code:
- group_content -> group_relationship
- group_content_type -> group_relationship_type

Also, I removed $max_length for the prepareText() function, cuz I don't believe that this is the correct solution. We need to use a fully indexed value. The indexed value is already truncated by the Embeddings data type plugin

HitchShock → created an issue.

Sorry @smustgrave, forgot to submit my comment there :)

Wrote an answer to the last comment in the MR

HitchShock → created an issue.

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

HitchShock → created an issue.

HitchShock → created an issue.

Hi @benjifisher
1. Fixed a small grammatic issue
2. About the empty email error message:

If a user enters an empt address and an invalid one, then currently they get the message about an empty address. They fix it and try again, and get the message about the invalid address. Let's list all the errors at once.

I really liked this idea and implemented it.
But I don't like the idea to make an "empty" message like this

The email address "" is not valid. Use the format user@example.com, and separate the addresses with a comma.

For me, this message is unclear/unreadable/not user-friendly. It is very difficult to correctly display an "empty" value. Using quotation marks for this purpose can only worsen the situation, as the user may start looking for something that is not there. A message like that really can confuse a user.
While the direct text that an empty email address value has been entered is clear and not ambiguous.

HitchShock → created an issue.

FYI. The function field_ui_form_field_ui_field_storage_add_form_alter() still exists in the code after ✨ Make field selection less overwhelming by introducing groups Fixed .
Atm this is a dead code cuz the behaviour/logic was changed.
Since this ticket is in fact a continuation of that epic, it is the best candidate to remove this useless function.

Updated MR.

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