🇵🇹Portugal @dxvargas

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.

MR created. Waiting for review!

dxvargas → created an issue. See original summary → .

I'm extremely confused with the MR that I see related with this issue. So many commits... why?

As I understand we simply need to add something like this to lib/Drupal/Core/Entity/entity.api.php:

/**
 * Modify the list of available entity reference plugins.
 *
 * This hook may be used to modify plugin properties after they have been
 * specified by other modules.
 *
 * @param array $plugins
 *   An array of all the existing plugin definitions, passed by reference.
 *
 * @see \Drupal\Core\Entity\EntityReferenceSelection\SelectionPluginManager
 */
function hook_entity_reference_selection_alter(array &$plugins) {
  // Remove a plugin.
  unset($plugins['broken']);
}

What am I missing?

I've created a MR.

dxvargas → created an issue.

This happened to me after messing around with composer update/require/remove/etc.
I've fixed it by deleting the folder vendor/commerceguys/addressing and running again "composer install".

I'm now submitting a new patch with some changes suggested by @huzooka in the previous comment.

1. Done
2. To do
3. Done
4. Removed
5. Comment removed. I got into a situation where the entity was not an instance of FieldableEntityInterface, for that reason I'm not using your suggestion.
6. Done
7. Done
8. Done

I'm uploading a patch with the approach given by @claudiu.cristea, with a hook whose invocation returns a list of entity types that should have the publication date.
It's far from being a final patch but it works in a basic way and may be useful for next steps.

I think this should be optional. I don't agree that this field should be enabled without asking, just by enabling this module.

I agree with @claudiu.cristea that creating files on the fly is not a good idea.

Also, I don't see the reason to do this. Can you please explain?
Why not to do the storage updates immediately when saving the config?
I've done this experience and it worked. I can submit a patch.

About the config page, I don't see a problem with that.
Using hooks would also work but I actually prefer using the configuration page.

I'll reopen the issue for the discussion to continue and the patch to be improved.

dxvargas → created an issue.

I cannot reproduce this error.
After so much time without any replies, I believe the best decision is to close this issue.
@skitten, Feel free to reopen with more details if needed.

I've tested it and read the code changed, it's fine. I'll mark it as RTBC.
Not sure if it's a bug but anyways it's a good improvement.

The work is good so far and it's solving the problem that is being reported. Good.

But the method "\Drupal\file_url\FileUrlHandler::urlToFile" is where the problem started. It declares that it always returns a FileInterface but it can also return NULL. And there are other usages of this method that can also raise errors because the NULL case is not being handled correctly.
I think this should also be fixed. Here or in another D.O issue. What do you think?

This problem was fixed already in #3125505.

My patch shows also the corrupted characters when opening it here in the browser. Just like the patch in #3.
However, after downloading it I see them correct and I can apply #3 successfully.

This patch #3 from @viren18febS is working correctly. I put the issue as RTBC.

Thanks for the patch @viren18febS but I still see the corrupted characters.
I'll try to add my own patch and see how it goes. Please have a look.

dxvargas → created an issue.

It makes sense to not validate an empty source field.
I've tested with Drupal core 10, 1.x-dev and using a multiple taxonomy term reference field. Before when adding a new item I had this error. After applying the patch the error was gone.

I've tested the patch with success.

It is similar with the code existing in the module group except that:

1. it applies the change of owner to all revisions and
2. validates the entity before saving (otherwise it would raise an error)

Right @glaze, I forgot to check that, stupid me!
I close this issue. Thanks!

I've checked and there are several modules that use the "administer site configuration" to control their configuration page access. Examples:

• Devel
• Admin toolbar
• Purge UI
• Ajax comments
• Chosen
• Compact Date Range Formatter
• SPARQL Entity Storage

There are also modules having their own permission to control the access to their configuration page.

I think, since the configuration of this module is very basic, we can rely on the generic "administer site configuration".

dxvargas → created an issue.

What is the unclear/ugly error message?

I had the same problem. I found out that it was caused by not having the PHP extension for redis.

I fixed it by running:
sudo apt install php8.1-redis

The tests are now fixed in #32, good!
For the rest, it is the same fix as before (#25), that was already reviewed by the community.
Let's move back the issue to RTBC.

Search API was already fixed and the issue here should be fixed when using the updated version.
I close this issue, feel free to reopen if needed.