🇵🇹Portugal @dxvargas

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.

Fix for issue #3477426 is also need, I'm adding as related issue.
Please review.

dxvargas → created an issue.

Here is the MR for 3.0.x.
https://git.drupalcode.org/project/private_message/-/merge_requests/174

I put a test in place for this. It's ready for review.

I did the changes for 4.x. But it still needs the tests.

The MR is working.
I'm not sure about removing the return type declaration, "?string" could be there. But since the return type declarations are not consistently used in the class and it's not used in the interface, removing is also an option that makes sense.
I mark the issue as RTBC.

dxvargas → created an issue.

The approach using javascript seems valid to me.
But there are some important lint errors in the MR that need attention. Please have a look to the result of the job.
A test for this is also welcome.

I've created a MR to remove the tag, please review.

dxvargas → created an issue.

I'm amazed that my patch #20 does not work and I cannot understand how the error can still happen.
If @webdrips or @emil-stoianov can please explain with more detail what happens, I would be greatly thankful.

dxvargas → created an issue.

I have another suggestion.

When a user 1 is participating in a thread and blocks someone in that thread, the user 1 will loose track of that thread, like if it didn't exist (it can only access to the thread if it uses the URL!!). It will also loose track of all the threads that the blocked user participates. It seems like the blocked user is the one that blocked someone!

I think the user 1 should continue to access all threads.
It just shouldn't read the messages from the blocked user.
But I think the best would be to see them obscured. I consider important to be aware that a message was posted in a thread.
Remaining tasks

It's also important to understand how the Passive/Active blocking works for threads.

When a user opens the messages page (/private-messages), the Reply textarea gets the autofocus.
My first suggestion is that the behavior should be the same, no matter we click in an item in the Inbox or we open /private-messages.

Usually the Reply textarea is next to the last received message. Receiving the focus seems acceptable.

But we can imagine, if there are several new messages, the user should see, in first place, the oldest of the new ones.
For this, we could add a class "private-message-unread" to unread messages. Then it is easy to focus the one we want.

dxvargas → created an issue.

dxvargas → created an issue.

Just passing by to say, I'm closing #3488248 ✨ Add the ability to disable the number if it is 0 Active as a duplicate, in favor of this one.
The only difference is that in the now closed issue, there was the suggestion for the visibility of the zero to be configurable.
Please have this in consideration and proceed as you think it's better.

I think this one can be closed in favor of #3486047 ✨ Do not show zero Active that was created before and it has already a patch. I'll comment there too.

I have enhanced the existing Functional JavaScript test testNotifications() to test a banned user.
Moving to Needs review.

I have created a MR with a fix.

But I let the issue in status "Needs work".
The tests are failing (but I don't think it's related with my changes).
We need to test also if like this the notifications by email are also prevented.
Maybe some tests?

I am also experiencing a similar situation. in my case, only with two users. When the banned user 2 sends a message to "banning" user 1, the user 1 gets the new message notification.

For now I think we should use the same approach as in the threads notification and filter out messages in threads that have the banned user.

I don't think we should remove the "banning" user 1 from the notifications. Because if that users removes the ban, he should get access again to the threads.

The user display name is being shown instead of the username everywhere (except user selection autocomplete as mentioned in the issue's description). The code is good with many nice improvements. I mark as RTBC.

For now I've implementing the proposed resolution and I'll wait for feedback. Moving to Needs review.

Other options:

1. Maybe Drupal.PrivateMessages.loadThread() could have as second argument the code to run on success. Like this we are not waiting for the result to highlight the new thread in Inbox. I think like this is the right way and also the full thread should be replaced with a timer while the new one is being loaded.
2. Maybe a better solution would be to implement a MutationObserver logic. Like that the Inbox block would automatically react to changes in the full thread.

dxvargas → created an issue.

I just want to confirm the RTBC.
The new version of the MR, after the last two commits managed to fix the problems in the pipeline I had before.

dxvargas → created an issue.

@webdrips it is impossible that you have the same error after applying the patch in #19.
There is only preg_match() method here and it receiving as second argument ($subject) this: $mergedParams[$token[3]] ?? '', and this is never NULL.

My patch in #19 fixes the problem.

About your patch in #20, I have the same problem that I had with the changes in the MR.
That patch changes the logic!

Before, if $mergedParams[$token[3]] is null, !preg_match('#^' . $token[2] . '$#', $mergedParams[$token[3]] ?? '') evaluates to TRUE and an exception is thrown and no $url is returned.
After your patch, if $mergedParams[$token[3]] is null, the processing goes to $url = $token[1] . $url; and continues.

A patch here cannot change the logic. If the logic is not good, please fill another issue and let's discuss that.
Here we just need to make the PHP warning go away. Patch #19 does that.

As I understand, no one here had any other problem except getting the deprecated warning. This is my case. Everything is working as expected except that I see this warning.

So, why not to just avoid this warning in the most simpler way possible?

This is what I understand that Symfony did. In the documentation, it says that the code is adapted from \Symfony\Component\Routing\Generator\UrlGenerator::doGenerate(). If we check the changes in this method, they are basically replacing $mergedParams[$token[3]] with $mergedParams[$token[3]] ?? ''.
Please check here the old code:
https://github.com/symfony/routing/blob/3.4/Generator/UrlGenerator.php#L...
And check here the new code:
https://github.com/symfony/routing/blob/7.1/Generator/UrlGenerator.php#L...

In my case (that I miss to reproduce in a vanilla install, thus I won't explain in detail) I get an error when I apply the patch in a piece of code that apparently has nothing to do with this.
I can briefly say that it's in a hook in private message entity (from the private_message module) that is displayed in layout builder, because the entity has no longer a owner. My code is heavily customized, it's difficult to debug the new error and explain what is happening.
Anyway, I think my case demonstrates how a change here can have unexpected impacts.

One think that looks suspicious in the current MR changes is that before, for each $tokens, the $url was being set with a new value.
With the changes in the current MR, if the variable $optional is true, the $url will remain unchanged.
I don't really understand all the logic here. But this doesn't look right.

I don't want to just change the MR in a completely different approach, so I'll just leave a patch here with the change I'm proposing.

I have a question about the patch #25 and the usage of Drupal\Core\Render\Markup::create. Is it safe?
I'm afraid we are marking the message sent by the user as safe and sending it as is in the email.
Can someone please confirm or rebut this?

I agree @zaryab_drupal, like this is better.

There are failing tests which are not related with this MR (I see them also in main branch).
I mark the issue as RTBC.

I've created a MR.

PS: I'm (not so) sorry to use the "@", I think no one likes it except me. I think here it's the shortest and cleanest fix. Feel free to add the much more consensual "?? NULL" instead, that's just about the same AFAIK :)

I've created a MR.

PS: I'm (not so) sorry to use the "@", I think no one likes it except me. I think here it's the shortest and cleanest fix. Feel free to add the much more consensual "?? NULL" instead, that's just about the same AFAIK :)

dxvargas → created an issue.

I've updated the validateForm return type following the changes in private module done in https://www.drupal.org/project/private_message/issues/3477873 🐛 Return type for PrivateMessageConfigFormBase::validateForm() should be void. Active .

Ideally, a release here will follow soon a new release in private_message.

I put this issue back to "Neews review".

The code seems good. I've tested in a vanilla install, ran the tests before and after... works as expected.

I can understand your preference @huzooka.
Not sure you've seen the related issue in #3477426 🐛 Missing Return Type for PrivateMessageFloodPrivateMessageConfigForm::validateForm() Active in Private Message Flood Protection. I still think we should move here and quickly adapt there.

But sure, let's wait for more feedback.

I've created a MR, waiting for review.

When this one is fixed, #3477873 🐛 Return type for PrivateMessageConfigFormBase::validateForm() should be void. Active , we'll need to update the return type of PrivateMessageFloodPrivateMessageConfigForm::validateForm(), to void. In the meantime, we need a return []; there.

dxvargas → created an issue.