🇬🇷Greece @dimilias

Please, note, that the homepage of the module is also up for review. I provided a shorter description for the homepage of the module.

Requesting review.

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

I have reviewed the patch and works fine. I do not like the redundant comments like "Log in the user" but I see that all tests are like that so it is fine to stay for consistency even if it is unumbered.

Sorry, this ticket needs to continue. Even if the tests pass, the UI is broken. I tried to click translate and I had an issue with some constructor signatures of the translation plugin and another issue with the source class.

There is one minor warning in the tests but is it coming from the webform module. Ignoring. Everything is ready to be merged.

Ready for review.

I have removed the optional token support due to https://www.drupal.org/node/3404140 → causing failures to the signature of the config form.

Since we do not have forms to approve or reject yet, I have implemented a hook mechanism to send the notifications on entity create when the entity is created as pending, and on update when the entity is pending and transits to active or rejected.

dimilias → created an issue.

Approved.

I am going to merge it. The logic is sound, the results are very good, I could not find a functional issue, and no clear problems in the code. Let's go with it.

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

Pretty straightforward. Resolves the issue.

@andras_szilagyi my comment from https://www.drupal.org/project/babel/issues/3542255#comment-16279797 ✨ Webforms handling Active is still happening

Merging so that the rest can follow. Thanks for the clarifications.

Attached some remarks. But also, I cannot seem to use it right now in the UI (there are relevant remarks).
I created a scope, created a request type, set permissions to authenticated user.
I tried to request a new client, and I don't see anything on my user profile relevant page, not in the clients, not in the requests.
I also tried to create one as a manager with the same results.
In both cases, the client was empty in the database request record - expected for an authenticated user, not expected as a manager.
There are tests to approve or reject, but I do not see a UI way to reach that point.

@grevil I wonder if https://www.drupal.org/project/config_inspector/issues/3548299 🐛 Missing schema on nested config Active would fix your case.

I did the following:

ddev start
ddev poser
ddev symlink-project
ddev restart
ddev install
ddev drush en -y babel_webform

Results:

$ ddev drush en babel_webform
The following module(s) will be installed: babel_webform, webform

 ┌ Do you want to continue? ────────────────────────────────────┐
 │ Yes                                                          │
 └──────────────────────────────────────────────────────────────┘

>
> In Container.php line 157:
>                                                                                                                       
>   You have requested a non-existent service "plugin.manager.config_translation.mapper". Did you mean this: "plugin.ma 
>   nager.config_action"?                                                                                               
>                                                                                                                       
>

In ProcessBase.php line 155:

  Output is empty.


Failed to run drush en babel_webform: exit status 1

I provided a patch but without test, because as I said, I am not sure on the solution.
Keep in mind that this has significant impact on the total time of the command (more than 60% up).

Setting to needs review to get some ideas.

Added the related issues. If their case is based on schema like block.settings.[id] (page_manager.block_plugin.*) and they have more than one config objects that are being validated for this, then probably they have the same underlying issue.

dimilias → created an issue.

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

Meh, I reached a point where I was writing tests for this, then I realised that this is kind of a double functionality.. We already have a way to limit down the available scopes, and this is by having a single permission per scope generated.

I suggest we do not do this.

dimilias → created an issue.

We have talked with @claudiu.cristea and decided in two changes:
1. Add some guidelines per plugin where they can set explanations and other information when the plugin is selected.
2. Add text wrap to the second column in order to allow text to be shown fine even when long text is inserted since there is no resizing in protected documents.

I have tested the above with LibreOffice, OpenOffice, MS Office.
The protection is only applied in XLS and XLSX file format and applies across all of the above software.
ODS protection is not applied. Not even for native software of the extension. Probably the library we are using does not support it. We discussed and agreed that it is fine and it is not a security measurement, rather than an optional convenience.
We have agreed that we should not remove the extensions though, because in many cases, the site owner might want to work with the Open Document format rather than some proprietary extension.

CSV of course does not carry any of these as it is mainly a plain text file.
The guidelines added for the Spreadsheet plugin is that we offer minor protection for the XLS and XLSX formats.

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

Everything seems to be working fine. Requesting review.

dimilias → created an issue.

Works nicely for the Ajax form.

RTBC. Thanks.

Merged into 1.x

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

Something went wrong with this in the meantime. Data are not tracked for me. Also, setting this to needs work since there were remarks.

Thanks all.

Fixed. Forgot to remove a table prefix.

Please, note, that this includes an update to ddev-drupal-contrib because I needed the ddev core-version command to debug the issue - and yes, I understand that I could do it with the environment variable but at some point, we should update this too.
Still, if you find this irrelevant, please feel free to reject or revert the commit.

There was an issue with the latest version of Drupal. After https://www.drupal.org/project/drupal/issues/2786355 📌 Move multiple provider plugin work from migrate into core's plugin system Active , tests that use the entity_test module, which has an implicit reference to the user module through the entity owner interface, will require the user module to be installed as well.
I have fixed this and the rest is working fine so I am approving this.

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

Thanks

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

We have discussed with Claudiu and out weighted the pros and the cons.

The task is updated to have a "Delete selected/Delete all" list of buttons.
No tests will be required.
We have decided to go with the custom form instead of the views due to:
* The footprint of the code is more or less the same (<100 lines diff) but the views version also have the view in the export which is another 500+ lines file.
* Views offer out of the box some stuff but all fields still need to be added via the views data. Please, note, that if that was the only thing, then views, naturally succeed in this because of the out of the box UI afterwards. However:
* We anyway have to write custom code for the paths. Even if we were to write all in the views and use aggregation, still it would be not an easy task to achieve the same result as requested in views (because of the multiple keys in the table).
* We anyway have to write custom code for the filters to achieve to be a select list of available types.
* We anyway have to write custom code for the delete selected/all buttons.

Thus, we are going to go with a custom form rather than a view. I am closing MR14.

Quick and simple. RTBC.

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

Starting again since the relevant ticket was completed.

Thanks. Merging.

Thanks for the work. The number of entries agree after the update.

Just for the record, the query from https://www.drupal.org/project/babel/issues/3536782#comment-16207723 🐛 Data integrity error: some hashes recorded as babel_source lack status in babel_source_status table Active point 4, becomes drush sql:query "SELECT COUNT(*) FROM (SELECT DISTINCT bs.hash FROM babel_source bs INNER JOIN babel_source_status bss ON bs.id = bss.id WHERE bs.plugin = 'config') wrapper" after the update as the hash is not there in the bss table and the plugin is in both tables (ambiguous).

Added one more module I have created that might be of use to some users.

Tested and does what the scope expects. Note, we have a follow up for further investigation. Thanks.

Thanks all.

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

I have tested the setup using the ddev drupal contrib package of ddev and can confirm that tests pass both for version 2 (D10) and version 3 (D11) of cas module. Thanks.

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

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

It seems like the culprit is because I can see in the babel_source table locale strings that do not have an ID

| plugin | id | status | hash | sort_key                  |
| locale |       | 1          | abcd | Announcements: |

Somehow, in BabelService, in

foreach ($rows as $row) {
      // Pickup the first key from $ids as the source string and context.
      [$key] = explode(',', $row->keys, 2);
      [$pluginId, $id] = explode(':', $key, 2);

      $byPlugin[$pluginId][$id] = $row->keys;
      $status[$row->keys] = $row->status;
    }

we are getting a locale: which results in an empty ID and the array_flip at the end of \Drupal\babel\BabelService::getStrings is causing it to be an integer - it tries to merge it with the $list but since there is no relevant match with locale:, it just retains the index as a value).

idimopoulos → created an issue.

Thank you.

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

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

Let's block this on https://www.drupal.org/project/track_usage/issues/3535768 📌 Replace JSON column with normalized table structure Active

I have further fixed the failing test, added the new hooks mechanism of D11.1+ and fixed codesniffs and phpstan.
Requesting one more review.

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

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

I will try to create a test but I cannot do it this week (sorry).
What I have found is that there is a generic error with the inline_form_errors and its attempt to create links to the fields.

The problem is when the constraint is added to the element rather than a property of the element e.g. (example from our code)

function ..._entity_bundle_field_info_alter(array &$fields, EntityTypeInterface $entity_type, string $bundle): void {
.
.  
    /** @var \Drupal\field\Entity\FieldConfig[] $fields */
      $fields['short_id']->addPropertyConstraints('value', [
        'Regex' => ['pattern' => '/^[a-z0-9-]{4,26}$/'],
      ]);
      $fields['short_id']->addConstraint('UniqueShortIdInsensitive');
    }

In the above, we are adding one property constraint, and one constraint on the whole element.
You may consider a case where you want all values of a multi cardinality field to be different even though above, we are trying to keep a unique value in the database.
Disclaimer: You may claim that the addConstraint method should be instead addPropertyConstraint. You are correct, it might be wrong design. However, it does not solve the underlying issue, as a constraint that is needed to be set on the element level is easily to imagine as I mentioned above.

So, there are 2 issues with the inline_form_errors. The first one was fixed (for us, though it needs tests and review) with my patch above. There was a problem that WidgetBase::flagErrors did not descend more than the delta, thus the ID constructed for the inline_form_errors, was edit-my-field-0 instead of edit-my-field-0-value. By default, only the input child is receiving an ID, thus the link from inline_form_errors which would point to href="#edit-my-field-0" would not lead anywhere. As mentioned, now it properly descends.

However, the main problem here, is with the element level constraints. By default, form-element.html.twig does not add an ID on the element level. So, the element level link from inline_form_errors will not point anywhere with the href="#edit-my-field".
The problem further expands because, while I did workaround it a bit on my project by trying to force an ID to the elements, there are some cases where this would create conflicts. For example, a select filter and a field with the same name, would have to produce the same ID, so I cannot force it for all form elements in any case without considering what else is in the page. So I have built some exceptions in our code.
But even further, \Drupal\inline_form_errors\FormErrorHandler::displayErrorMessages seems to detect the wrong ID anyway, so if I set it to the element level, it will still add the fragment edit-my-field even if I add a custom ID, so the above cannot be fixed with the \Drupal\Component\Utility\Html::getUniqueId as it might make it edit-my-field--2 but the fragment will not change accordingly.
The complexity (not complaining about the complexity) of the element -> widget -> delta -> property of the form API does not seem to be respected from the inline_form_errors.

An easy way to reproduce this (I have to write the test with the inline_form_errors module and a custom constraint) is to simply create a constraint, add it to an element with the field_info_alter hook, fail the constraint, check the link from inline_form_errors, and confirm that the fragment does not match the ID.

Mainly, from what I could see, many widgets that represents element with a single property (like the mainProperty is just a single one like value or target_id), they often tend to return that property as part of the \Drupal\Core\Field\WidgetBase::errorElement. This is why it does work for some fields. For others, that simply return the element, more specific targeting is required.
I have created a patch that tries to descend further into the element in order to try and pinpoint the exact property to add the violation to, according to the property path. It also further checks whether the error element is already returning the appropriate target.

Note, that for this to work properly, a violation constraint needs to be added properly as well, i.e. using the ::addPropertyConstraints instead of the generic addConstraint.

The initial MR seemed to have broken multiple tests which revealed the second point about the ::errorElement already returning the correct property from widgets.

I am still not sure where to write tests for this though :/ any ideas? because it would be easier to write it as a FunctionalTest using the inline_form_error which adds links that can be clicked (that reveals the problem) but seems too much for a widget base. But a PhpUnit test would require quite some mocking (?).

I am adding the label needs tests.

Hiding the patch files and setting this to be a bug in the field system instead.

You are correct @gordon. It seems to be a more generic issue of the form errors method. I am going to work on it for a bit and try to write a test on this. Setting this to needs review only for an initial test run.

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

I have rerolled the patch and added a test for each case.
However, I extended the MR to also include an update to the location hash. I don't see the point of having the ability to work when manually adding the fragment but not being able to see it when clicking the tabs.

Disclaimer though: Feel free to disagree and remove the last commit.

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

Works as expected, resolved the issue. And the fix is minor and straight. RTBC+1

idimopoulos → created an issue.

idimopoulos → created an issue.

I agree that this is kind of too business logic related.
This adds generic properties to the block (correct) but it adds specific logic to the code with some hardcoded values (like the etranslation).
I would say that this would be better if it would go generically in TMGMT module with the following:
* Provide the possibility to show messages.
* Select manually which plugins this message is shown for
* Provide the messages and the type
* Maybe additionally allow to select the languages to show the message for.
* Explain in a help text something like "this can be useful if for example you want to inform the user that this is a machine translation or that translations might not be accurate in some languages"

Tested and works as delcared. RTBC+1

I am sorry @smustgrave. The original branch was not applying cleanly to 11.x, but was to 11.1.x. So I thought this was the target now. I have created the 11.x branch.
As for the 10.5.x, I created it for our project mainly as the original branch's patch, was not applying cleanly.

idimopoulos → changed the visibility of the branch 3516691-getimagesize-failed-to to hidden.