🇵🇹Portugal @dxvargas

The motivation seems legit. The implementation is simple and correct.
Marking as RTBC, I hope it will be welcomed.

dxvargas → created an issue.

dxvargas → created an issue.

I've tested as much as I could. Added another test. Review please.

I need to cover some edge cases. For example, when the original translation was empty.

I've addressed the remarks in #8:

1. I've adapted the code to keep the changes done by the user.
2. I've added tests.

Looking forward for reviews!

I'll give it a try with the goal explained by @claudiu.cristea in #8.

Answering to @claudiu.cristea in #8.

I think we need both, to keep the input on "Filter" and also to bring updates from outside. In case of conflict the input should win (not 100% sure, but it seems the correct resolution). And we need to test this.

If we have a translation in the form input that is different from the storage, how can we decide which one is correct?

1. Maybe the user has changed the translation in the input, then the input value should be kept.
2. Maybe the translation was changed outside the form, then the input should be updated (that's the goal of this issue).

To make the distinction between these two possibilities, we would need to keep an hidden input with the original translation.
If this original translation is different from the received translation and different from storage, we know we are in case 1.
If this original translation is the same as the received translation and different from storage, we know we are in case 2.

But I think this is too complex.

I propose an other behavior that I expect that is easier to implement and to understand:

• If the user saves a record, we don't automatically update any input in the form.
• If the user filters the list, every record is automatically updated with the values from storage. In that case, any unsaved translation is lost.

I think this simplifies the process and it's an expected behavior. It complies with the initial plan from @huzooka, explained in #6, and also with the description in the current issue.

I've made a small change that fixed the issue.
Ready for review.

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

The code is good and it works correctly. I mark it as RTBC.

@dieterholvoet with the changes here in .gitlab-ci.yml, we are testing with Drupal 10, for which the PHPUnit attributes don't work. Still, all the tests are passing. I would expect that also happens for Drupal 9.

Other remarks... Maybe it would be a good principle, to test with all supported Drupal versions.
For PHPUnit tests, even if they are not running successfully in Drupal 9, it doesn't mean that will cause issues, unless someone wants to run the module's tests in Drupal 9. But usually these tests are mainly used for module's own development.

I was dealing with the issues in pipeline regarding PhpUnit using max PHP version.

The tricky part is that there are a lot of deprecations affecting TMGMT and there is not much we can do about that.
Actually just a few of the deprecations are problematic, the ones that are considered risky.

I've tried many strategies, in the end I've chose to use a similar strategy that was adopted in webforms, to allow to fail:
https://git.drupalcode.org/project/webform/-/blob/6.3.0-beta4/.gitlab-ci...
But here I chose to run with the option "--do-not-fail-on-risky". It works and I consider it more targeted to what we need.

By the way, tmgmt_deepl → has the same problems with TMGMT in PhpUnit tests with PHP 8.4. It seems that this is a generic problem for which there is no good solution.

I ask for review, maybe someone else has better ideas.

I'm reverting the decision to delete translations when they come empty in an imported file.
Empty translations in an imported file will be ignored.

About the test module "babel_test_config".
I took a strategy to have initial sources for translations by enabling the new test module "babel_test_config". This seemed a clean and fast way. But this is not working properly in previous versions of Drupal core and I ended up needing a workaround with a comment "@todo Remove after compatibility with Drupal < 11.1 is dropped.".
Maybe there are better ways to have initial sources for translations. But I'll wait for feedback in reviews.

I've prepared a MR fixing this issue, it's ready for review.

In the process, I'm also doing another important change.
Before, when submitting a file with a translation item with an empty translation, it would be ignored.
After, when submitting a file with a translation item with an empty translation, it would remove the translation from DB.

I move back to Needs review after my reply.

[@alorenc] about your remark:

I am not entirely sure about the additional condition: "or if no active alerts" option."

This condition, and the associated logic, was already there. I just moved it up and used the cache also in that case.

This comes from the related issue:
#3480439: Unnecessary invocation of sitewide_alert/load route when no active alert exists increases server load
I've added a comment there before. I think it's debatable if the change there makes sense. But it's a different topic.
Here we should keep the existing logic.

dxvargas → created an issue.

I want to mention that the present MR changes the way this module works, because now the alert is only displayed after the user loads a page. Before, it would show even without a page load.
These lines change the logic. If there are no alerts when a page is loaded, no javascript will be included and no calls to the server checking if there are any new alerts.

I could save drafts and editing them after correctly.
I made two small change requests, so I reopen the issue.

I'm providing a patch that should fix the problem. When saving alerts, it's invalidating the new cache tag "sitewide_alert", that is later used in the build of the render array.

We always need to apply the cache metadata to the build, otherwise the cache doesn't bubble correctly to the alert block in the submodule. This led to some refactoring of \Drupal\sitewide_alert\SitewideAlertRenderer::build().

I'm providing a functional test that was already passing before applying the fixes!

This problem should be fixed by now, feel free to reopen and provide more information if needed.

dxvargas → created an issue.

When installing the site on clean, there are no menu_link_content rows in the babel_source_instance table.
This means that the described bug doesn't exist. When reproducing the steps, we have 0 such rows in the second step and 0 such rows in the final step.

In \Drupal\babel_menu_link_content\BabelMenuLinkContentService::batchAddSources() we can see that only custom menu links (menu_link_content) are processed. But after a clean install there are no custom menu links. Like this, adding or removing menus, will have no effect; no rows are added or removed.

If we add a link through the Drupal administrative interface to a menu, then we can see that the rows will change (by 1) when enabling or disabling this menu in the "Menu Link Content" config.

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

Merged, thank you all!

I agree with this feature request.

The MR of the issue #3433991 already has the proposed changes regarding phpstan and phpcs.
I will close this issue as duplicated.

The MR is green now, after merging 2.x into the branch from here.
Review please!

The issue https://www.drupal.org/project/private_message_flood/issues/3501011 📌 Private message 4 compatibility fixes Active is now merged.

MR is merged, this issue is now fixed.

This feature request is useful and it's implementation is working. I mark as RTBC.

dxvargas → changed the visibility of the branch 2940605-11.1.x to hidden.

The MR4994 is re rolled for 11.x and it's green.
I hope we can merge it soon and back port it to Drupal 10.

Not sure about this but I've changed the deprecation notice to removals happening in Drupal 12 (after reading #177 and because Drupal 11.0.0 that was being mentioned is now old).

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

The test is fixed. It had an unrelated violation ( contains a translatable label, so a `langcode` is required, resulting in a violation of \Drupal\Core\Config\Plugin\Validation\Constraint\LangcodeRequiredIfTranslatableValuesConstraint).
Moving this issue to Needs review.

The changes in the first commit of the MR here are the same as the changes already done in core after the MR in #3533926.
Like that, the only changes that we can keep here are the introduction of a test.
I'll adapt the title of this issue accordingly.

The new test is failing, so this still needs work.

The MR here has conflicts after https://www.drupal.org/project/drupal/issues/3533926 🐛 Config of the type plural_label can never be valid due to the label constraint Active .
Adding this related issue here.

The MR is green. Ready for review.

dxvargas → created an issue.

Changes are correct, marking as RTBC.

The module parameter was removed here for good reasons:
https://www.drupal.org/i/2443605 →

Then it was reintroduced here, but as I understand it wasn't well explained why we need this field.
https://www.drupal.org/project/tour/issues/3468446 🐛 Dependency calc seems to be removing all items Fixed
I think it was reintroduced to make the UI to continue to work. But there was no discussion why we need it back, no arguments against the points given in #2443605.
Also, now the schema doesn't have the module mapping! When I re export a configuration, the "module" info is removed from config.

I think the way to go is to remove again the module entry from the Tour class. And to remove it from the src/Form/TourForm.php, following the path that began in #2443605.

The MR has fixes for the problems in phpstan, phpcs and eslint. It's is ready for reviews.

Reopening. I will fix the problems in phpstan, phpcs and eslint for all tests to pass.

+1 release please!

The changes are correct, I mark the issue as RTBC.

@dieterholvoet, OPT_IN_TEST_PREVIOUS_MAJOR is not being removed, it's moved some lines under.

The changes are correct, marking it as RTBC.

The changes are correct, marking it as RTBC.

I've added gitlab-ci file here but there are many changes required for phpstan, phpcs and eslint to pass. That is why I'm marking them as not required and I propose that this is fixed in a new issue. I'm not sure it's the best strategy but I'll wait for a review.

The changes are correct.

I have done some improvements and the MR is ready for review.

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

I did the change suggested in the comment by @claudiucristea.

I have created a MR with the change to make HtmlLink to respect the FilterUrl settings, with test coverage.

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

The code to focus the tab with errors should happen inside a "once", to not run after AJAX events.
This is basically the main change I am proposing.

The focus of the tab with errors is covered already in \Drupal\FunctionalJavascriptTests\Core\Form\FormGroupingElementsTest::testVerticalTabValidationVisibility() and it is passing in my MR.

I have a simpler use case where paragraphs is not needed, just Field group. It can be replicated in latest D11.
I'm attaching an image with the configuration bellow.

1. Go to the "Basic Page" content type
2. Make body required
3. Add an image field
4. Go to the form display page
5. Add vertical tabs "Tabs" with tab "Tab 1" and tab "Tab 2"
6. Put "Tab 1" and "Tab 2" inside "Tabs"
7. Put "Title" and "image" inside "Tab 1"
8. Put "Body" inside "Tab 2"
9. Go to the Basic page creation form
10. Fill the title and submit
11. The form is submitted and there will be an error because Body is empty. The visible tab will be "Tab 2" by now, this is correct.
12. Go to back to "Tab 1"
13. Upload an image
14. The tab "Tab 1" is hidden and we jump to "Tab 2" with the body. This should not happen.

The problem exists whenever:

1. A validation error exists in field A
2. A field B that updates the DOM of the page is changed
3. The field A and B are in different tabs.

The problem comes from the core file web/core/misc/vertical-tabs.js.
When an image is uploaded (or removed), there is a change in the DOM. Then, every time there is a "DOMContentLoaded" event, the context.querySelectorAll() in Drupal.behaviors.verticalTabs is triggered and it looks for tabs with errors and programmatically clicks them. This is what makes our tab that has the body to be visible.

What I read in the link that you've mentioned (this one: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...) is that this hook provides field definitions for a specific bundle within an entity type. And the parameter's documentation is string $bundle: The bundle., not leaving the option for it to be NULL.

What I read in the link that you've mentioned (this one: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...) is that this hook provides field definitions for a specific bundle within an entity type. And the parameter's documentation is string $bundle: The bundle., not leaving the option for it to be NULL.

I didn't reproduce the problem but just by looking at the code, since getTranslateRoute() can return NULL, I will mark this issue and it's MR as RTBC.

I have tested the MR and it's working.

I have tested with 3.0.x-dev. The error is there, the fix works with that version and I've also ran successfully the test.
So, the MR should be targeting the latest dev version.

I think the code is a bit confusing, I'm suggesting a small improvement is the MR.
Anyway, for such a small edge case, it's quite a big change.
I think it's ok but if another reviewer has the possibility to have look, it would be welcome.

It works flawless! Code and tests are good.
I mark the issue as RTBC.

I have tested the patch and it fixes the problem for malformed YAML (e.g. "this: is: invalid: YAML").
This part is good.

When testing I've initially filled the field "Wrapper custom attributes (YAML)" with just a dummy string "BLABLABLA".
For this, the patch is not working. There is an error that is only visible in the browser's console:

TypeError: Unsupported operand types: array + string in Drupal\\webform\\Element\\WebformElementAttributes::validateWebformElementAttributes() (line 185 of modules/contrib/webform/src/Element/WebformElementAttributes.php). call_user_func_array(Array, Array) (Line: 281

Can we make this dummy proof and also display an error when a random string is filled in these fields?

I've created a MR with the approach proposed in #3.

But without the change being an option in the configuration.
I don't think that deleting the fields already filled when going back should be an option, never.
Also, I think it complicates the configuration page. The helper text in #3 says: "Hide the other steps fields instead of removing them from the current step.". Someone reading this for the first time wouldn't have a clue about the implications.
There is a good explanation in the code:

// Comment about the added use_hidden_flow:
// The #access = FALSE is used to do not add the element
// on other steps. As a sideeffect, the altered elements
// do not persist if the back button is clicked.
// To preserve the updated values even when clicking back
// the #access = FALSE code is removed and replaced by a
// hidden class. All the elements will be there, but will
// be hidden to the user.
// The use_hidden_flow is a new configuration set at step
// level, so the new code will just affect those steps with
// the use_hidden_flow set to true, by default is false.

But putting all this explanation in the configuration form would make it too complex.

If this goes just like that it may raise backward compatibility issues in some sites that for some reason test the "#access" property of the fields. So, I propose to merge this in a major release.

If as proposed in #3 the usage of "#access" is configurable, then it won't have backward compatibility issues.

Ok, I've found a problem when making the "Back" button behave like "Next": the validation will work.
That is a problem. For instance, if the user is going back, this will force the fields to be filled and prevent him from going back otherwise. Not good.

This seems to me like a bug and any solution shouldn't be optional.
The described problem doesn't happen when clicking "Next"? Only when clicking "Back"? What is the difference? Why Next works and Back does not?

The patch in #3 is fixes the problem you've described but I'm not sure it's the best option.
I've tested in a entity reference field and in that case it doesn't fix the problem.

Still, I want to confirm that there is a bug when using Drupal core prior to 11.2 (when the new hook "entity_duplicate" is not run).

It happens when we duplicate a new introduced paragraph (with nested paragraphs).
In that case the new condition !$item->entity->isNew() is FALSE and the nested paragraphs are not duplicated. They end up being used in the original and in the duplicated paragraph.
Sorry that I can't provide a test ATM.

Just removing the new condition !$item->entity->isNew() fixes this.

This problem is not happening when using drupal core 11.2 In this case, the \Drupal\paragraphs\Hook\EntityHooks::duplicate is doing the job.
@peterwcm, @arunsahijpal can you please tell us if you're using a version of drupal prior to 11.2?

The code seems simple and correct. I've used the MR for a patch and it's working correctly.

MR is green now. Asking for review...

I've created a MR but it needs to be checked because it has comes errors in the pipeline.

dxvargas → created an issue.

Thanks for the review @alorenc! I've updated the MR with your suggestions.

If I log in again inside the second tab, the system will no longer update messages in the first tab.

Yes true, but the user sees error messages, so the fact that the page is not working shouldn't be a surprise.

Maybe the error message could be improved. Not it shows " Oops, something went wrong. Check your browser's developer console for more details.". But I would first fix this problem and then improve the UI if necessary.

I'm proposing a minimal change that does the trick. This situation is not common to happen, so I don't think we should put too much code for it.

Actually my proposal seems like what should be there is first place. If there is an error it doesn't make sense to continue the javascript routines.

dxvargas → created an issue. See original summary → .

I have created a MR with some fails but (as I understand) they are unrelated and the MR can be merged.
Asking for review.

dxvargas → created an issue.

dxvargas → created an issue.

Thanks for the review @claudiucristea !
I've updated the composer requirements, pushed some changes and replied to your comments in the MR.

dxvargas → created an issue.

Yes @claudiucristea, I confirm we need a backport for 3.0.x.
Teh code is pretty much the same, in this regards.

dxvargas → created an issue.

I've closed #3477426 as a duplicate. We can make all updates here.

I'm closing this one in favor of #3501011.