Fixed Hard Coupling (com_users | plg_users_joomla)

[carlitorweb]

Pull Request for Issue #20249

Summary of Changes

Added onContentPrepareForm event to the plugin for doing the form require alterations to the passwords fields.

Testing Instructions

Go to the administration and create a new user. Check the form that is shown need have the passwords fields with the attribute "require" if the plugin User - Joomla! have the Notification Mail to User option as No.

Expected result

Passwords field with the "require" attribute

Actual result

Passwords field with the "require" attribute

Documentation Changes Required

No

[zero-24]

hmm IMO this check can be dropped as if we get here without a valid form something else is broken anyway. I have not checked it but IIRC $this->_subject->setError('JERROR_NOT_A_FORM'); would not work anyway.

[carlitorweb]

if we get here without a valid form something else is broken anyway

I thought same, but I saw we use it inside another plugin, and no wanted to drop something without knowing the "exactly why"

[carlitorweb]

Done @zero-24 and thank you

[zero-24]

Please use __DEPLOY__VERSION__ this way the release script replace it with the correct version where that method get committed in :)

[Quy]

Align. Add one more space.

[zero-24]

I have just tested this and found two issue. In case there is a validation error $data is no stdClass but just an empty array. And the second is what happen when the plugin is disabled. Currently the password field is not required. But it needs to be required than.

How to reproduce (First Issue)

• set error reporting to max
• create a user with the username test
• try to create another user with the same username
• error pops up.
• when you debug you notice that the $data array is empty.

How to reproduce (Second Issue)

• Disable the plugin
• try to create a user
• pw field is not required.

How to solve the issues

• change default in com_users XML file that the pw's are always required
• restructure the plugin check so it disable the required status if running.

[carlitorweb]

Disable the plugin

@zero-24 if you disabled the plugin, others things like the login logic will break, among other things. In fact, there is a string warning about this. In case this plugin is disabled because another plugin will handle User synchronization, then we no need more the "Notification Mail to User", and we no need check for this.

try to create another user with the same username

We can add an is_object() check inside the condition for avoiding this

[carlitorweb]

I do not understand why the drone fail here?

[zero-24]

In case this plugin is disabled because another plugin will handle User synchronization, then we no need more the "Notification Mail to User", and we no need check for this.

Well my point is that the password fields are than not required in that case ;)
So my suggestion was to change the logic so the fields are required allways but not required with the setting is enabeld ;)

But I'm also fine merge this without that change and when this got accepted i do a PR that would handle the proposed solution.

[carlitorweb]

@Quy can you give me a hand here? This code is marked as a conflict for the merge, was moved to the plugin for solved the issue. Or maybe I understand wrong this.

[Quy]

I think you have to update your branch since there were changes to administrator/components/com_users/models/user.php 4 days ago.

[carlitorweb]

Thank you @Quy

[SharkyKZ]

It's __DEPLOY_VERSION__ (one underscore between words).

[SharkyKZ]

Change mail_to_user param fallback value and comparison value to be integer and comparison to be not type-safe:
$this->params->get('mail_to_user', 1) == 0 or !$this->params->get('mail_to_user', 1).

[carlitorweb]

Thank you @SharkyKZ

[Quy]

Create a duplicate user.
An error message is displayed.
Password and Confirm Password are no longer marked required.

#14851 and #20313 could be related to this issue.

[carlitorweb]

Done, thank @Quy

[mbabker]

Really shouldn't make a reference to a specific line number in a code comment. If the code's updated, this reference goes away.

[carlitorweb]

Is true, thank you @mbabker my mistake, sorry

[SharkyKZ]

The first comment should also be updated. $data is an empty array on save. After returning from error, $data is an array but populated.

@mbabker can you comment on form data issue? FormController does not load form data by default, making it unavailable in onContentPrepareForm when saving. Code based on Form::getValue() also fails. Is this correct behavior? Should we use data from input instead?

[Quy]

Related issue #17700 about empty $data.

[carlitorweb]

This is the correct way then to do this or need a different way?

[SharkyKZ]

The code works fine now.

Still thinking about @zero-24's suggestion to make password fields required by default. It would make sense not to auto-generate passwords when plugin is disabled, when mail to user option in plugin is disabled or when sending passwords is disabled in com_users.

[carlitorweb]

Ok, let do the suggestion of @zero-24

[brianteeman]

It never makes sense in the admin to make creating a password required.

[mbabker]

It never makes sense in the admin to make creating a password required.

For creating a new account, I'd say it would make sense to default it to required and conditionally unrequire it based on the other settings. Otherwise you're left in a spot where admin creates an account without setting a password and the new user's only way of getting into the account is through the "forgot my password" interface. I don't think that's necessarily the best new user experience, but we all know what opinions are like.

[carlitorweb]

This is a valid argument imo #20247 (comment)

[ghost]

@carlitorweb are the Test Instructions up-to-date?

[carlitorweb]

@fran so far, yes

[ghost]

"Notification Mail to User"-option is "No", both Password-Fields have a "*". I'm confused about the Test Instructions.

Also "Expected" and "Actual Result" have the same Description.

[SharkyKZ]

@franz-wohlkoenig Behavior before and after patch should be the same.

[SharkyKZ]

I have tested this item ✅ successfully on fe03395

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20275.}

[Quy]

I have tested this item ✅ successfully on b633bbf

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20275.}

[Quy]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20275.}

[HLeithner]

Thanks