Please rebase on 4.1-dev

Please rebase on 4.1-dev

It's just a small extension, a new feature might be a bit exaggerated. Or is there some other argument?

I have tested this item ✅ successfully on 963ad7d

I have tested the patch and it works perfectly

I have tested this item ✅ successfully on 963ad7d

Tested successfully on Joomla 4.0.0-beta5-dev and PHP 7.3.22-1

Thank you!

i'll not set this PR as RTC despite 2 good tests cause of #30834 (comment)
i'll let the final decision to RL's

We might as well all go home if this wont get merged

Some checks were not successful: continuous-integration/drone/pr — Build is failing

What does this mean? Action?

[EDIT] I see that this problem has resolved itself.

We might as well all go home if this wont get merged

We have 27 release blocker and most of the time I only see some new stuff if no one fixes the release blocker and only add new fancy stuff then you are right and we can go home.
https://github.com/joomla/joomla-cms/issues?q=is%3Aopen+is%3Aissue+label%3A"Release+Blocker"

We have 27 release blocker and most of the time I only see some new stuff if no one fixes the release blocker and only add new fancy stuff then you are right and we can go home.

I will close this PR and make a new PR based on [4.1].

You don't need to, I changed your target branch to 4.1-dev

You don't need to, I changed your target branch to 4.1-dev

I see it has already happened. 😃

Thanks.

Please rebase on 4.1-dev

Please test in Joomla [4.1].

@maikol-ortigueira / @pabloarias

I have tested this item ✅ successfully on 91ccc4f

I have tested this item ✅ successfully on 91ccc4f

Nice addition @sandewt. Too many times have I not realised I was logged in or out when wanting to be the reverse after clicking the button in J3.

Testing scenarios all worked.

Please add Documentation Required tag so that it can be noted to go back and review relevant help and JDocs.

RTC

I really like the feature (good idea) but I'm not sure about the parameter tbh. Is it needed at all? Perhaps we should (in a future version) go for a message management extension where all this notifications can be managed on one point?
Any thoughts?

What kind of message management extension are you looking for? Shouldt a module / plugin / component take care of its own messages?

Something like the mail templates?

Yes exactly.

I have tested this item ✅ successfully on 91ccc4f

I have tested this item.

I have tested this item ✅ successfully on 91ccc4f

Its showing a message on login and logout on frontend.

Thanks all.

Again: I would merge it, but I think the parameter are not necessary. We should avoid to add parameter for each thing (yeah I know we did it in the past very often...). If someone want to disable the messages, he/she/it can just make a language override to an empty string.

Additionally I don't think it's necessary to check if user is really logged in, otherwise the login/logout would have returned before, so you can assume, they are.

Could you adjust the PR for this?

Again: I would merge it, I would merge it, but I think the parameter are not necessary.

Sorry, I read about this. Do you mean both parameters?
For now I would like to leave it that way, until there is a centrally regulated message system. Or if several want it that way.

Additionally I don't think it's necessary to check if user is really logged in, otherwise the login/logout would have returned before, so you can assume, they are.

You mean?

Change this code:

// Show a message when a user is logged in.
if ($login === 1 && Factory::getUser()->get('id') > 0)

to:

// Show a message when a user is logged in.
if (ComponentHelper::getParams('com_users')->get('frontend_login_message', 0))
{

Do you mean both parameters?

Yes both parameters.

If someone want to disable the messages, he/she/it can just make a language override to an empty string.

I have the following notes:

1. Not everyone likes a login / outlog message and can get irritated by it. Hence the default parameters are hide.
2. Especially novice users of Joomla are often unfamiliar with overrides.

Additionally I don't think it's necessary to check if user is really logged in, otherwise the login/logout would have returned before, so you can assume, they are.

Done. Deleted check if user is logged in

I have tested this item ✅ successfully on ce1bf61

It's working fine.

I have tested this item ✅ successfully on ce1bf61

@sandewt please remove the parameters they are unneeded and using the override component should be possible with documentation thx

Needs some change as stated above.

Needs some change as stated above.

Removed: ? ? ? ?

See #30834 (comment)

Back to pending.

Back to pending.

@HLeithner and @bembelimen

Deleted the two parameters.

Please test.

• Install the patch.
• Login and logout and see the messages.

That's all, the options have been removed.

I have tested this item ✅ successfully on 61634a3

The messages are shown after login/logout

I have tested this item 🔴 unsuccessfully on 61634a3

I am sorry but after applyingg the patch I do not see the messages

I am testing on 4.1.0-rc1

Some system info:

Database Type mysql
Database Version 10.4.19-MariaDB
Database Collation utf8mb4_general_ci
Database Connection Collation utf8mb4_general_ci
Database Connection Encryption None
Database Server Supports Connection Encryption No
PHP Version 7.4.20
Web Server Apache/2.4.48 (Unix) OpenSSL/1.1.1k PHP/7.4.20 mod_perl/2.0.11 Perl/v5.32.1
WebServer to PHP Interface apache2handler
Joomla! Version Joomla! 4.1.0-rc1 Release Candidate [ Kuamini ] 18-January-2022 18:00 GMT
User Agent Mozilla/5.0 (X11; Linux x86_64; rv:94.0) Gecko/20100101 Firefox/94.0
---------------

I am sorry but after applyingg the patch I do not see the messages
I am testing on 4.1.0-rc1

I have no explanation for this.

Can you test with Joomla! 4.1.0-dev Development [ Kuamini ] ?

[EDIT] I have tested successfully on: Joomla! 4.1.0-rc1 Release Candidate [ Kuamini ] 18-January-2022 18:00 GMT

Hi again.

I am sorry but could not test the patch successfully.

I took a fresh install from https://developer.joomla.org/nightly-builds.html
I configured the patch tester but for some reason the oldest pull id available to me was #36134

I then tried to apply the patch manually but I did not get the expected result.

Best regards,

Berta

I have tested this item ✅ successfully on 61634a3

Hello:

My currently installed Joomla! version is ‎4.1.0-rc3-dev. I'm using PHP 8.0.15.

I've created a menu item to login and another to logout.

Before apply path: it doesn't show messages after login or logout.

After applying the path: it shows a message in both cases. After login and logout. Via module or menu items.

Thank you!

I have tested this item ✅ successfully on 61634a3

rtc

Thanks all. 😃

Oops, I see that this feature is not yet added to J4.1.
So, I have merged this branche.

@richard67

Why is this PR not merged to the core yet?

@sandewt No idea. I am not the only maintainer.

This change should go into 4.2. I changed the base branch, but now there are some commits added to your pr which will disappear when 4.1 got fully merged up into 4.2.

Additionally I don't think it's necessary to check if user is really logged in, otherwise the login/logout would have returned before, so you can assume, they are.

Done. Deleted check if user is logged in

@sandewt I see the check has been deleted in the login method but not in the logout method. Does that have a reason, or was that just missed?

@richard67 A good comment.
I don't know if there is a check somewhere in Joomla if the user is really logged out at that time. I'll go for safety then.

@richard67 A good comment. I don't know if there is a check somewhere in Joomla if the user is really logged out at that time. I'll go for safety then.

No need to be "safe" here, as if the logout failed, it would have been caught before. Although the check would be: if ($app->getIdentity()->guest) (we shouldn't introduce deprecated code) but as mentioned it's not needed.

@bembelimen @laoneo Do you think we need new tests after the last 2 commits?

I think one test for confirmation is enough

I think one test for confirmation is enough

@laoneo I think not

echo Factory::getUser()->get('guest') . '<br>'; // let's see 1
echo Factory::getUser()->get('id') . '<br>'; // let's see  0
echo $app->getIdentity()->guest  . '<br>'; // let's see 0
echo $app->getIdentity()->id; // let's NOT see 0

Are you also talking about the human tests for this pr?

I was indeed only talking about human tests.

I think this PR is not successful for the above reasons. Please test.

In summary:

When using this method, the user IS logged out (there is a logout message).

if (Factory::getUser()->get('guest'))

And when using the following method, the user is NOT logged out (there is no logout message).

if ($app->getIdentity()->guest)

Is completely unclear to me.

Just make it easy: remove the if... messages are enough

Just make it easy: remove the if... messages are enough

The moment the message 'You have been logged out.' is generated, the user is actually still logged in.
This is equivalent to flying the flag before finishing.
Of course, somewhere in the logout process, things will be checked, but still.

That's why I'm hesitating to drop the if statement. Give me a good reason to remove it.

@sandewt

The moment the message 'You have been logged out.' is generated, the user is actually still logged in.

That is not the case right, the logout is called before that on line 167
https://github.com/joomla/joomla-cms/pull/30834/files#diff-d3df0595ae1fa944f7270d2b86a20ae9358c93d7206c2191da23aebd8ce585a0R167

Do you want to figure out the discrepancies between getting the guest status?

Your comment here #30834 (comment) may need some further investigation in a separate issue.

This pull requests has been automatically converted to the PSR-12 coding standard.

Back to pending (RTC was added back by the bot by accident).

Thanks

Your comment here #30834 (comment) may need some further investigation in a separate issue.

@drmenzelit

Something went wrong here. This PR is merged to joomla 4.3. That shouldn't have happened.
Because this PR is not working.

Removing the if statement makes this PR work. The question is whether that is desirable. See the discussion above.

Change this:

// Show a message when a user is logged out.
if ($app->getIdentity()->guest) {
    $app->enqueueMessage(Text::_('COM_USERS_FRONTEND_LOGOUT_SUCCESS'), 'message');
}

To:

// Show a message when a user is logged out.
$app->enqueueMessage(Text::_('COM_USERS_FRONTEND_LOGOUT_SUCCESS'), 'message');

Please test #39048

@sandewt Please test #39048 . Thanks in advance.

@richard67

I'd like to do that, but then I'm actually testing my own code proposal. I wonder if that's desirable 😉