Fixed Hard Coupling (com_users | plg_users_joomla) by carlitorweb · Pull Request #20275 · joomla/joomla-cms

carlitorweb · 2018-05-01T18:07:52Z

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

plugins/user/joomla/joomla.php

zero-24 · 2018-05-01T18:24:49Z

plugins/user/joomla/joomla.php

+	 */
+	public function onContentPrepareForm($form, $data)
+	{
+		if (!($form instanceof JForm))


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.

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"

plugins/user/joomla/joomla.php

carlitorweb · 2018-05-01T18:37:20Z

Done @zero-24 and thank you

zero-24 · 2018-05-01T18:40:22Z

plugins/user/joomla/joomla.php

+	 *
+	 * @return  boolean
+	 *
+	 * @since  3.8


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

Quy · 2018-05-01T20:13:16Z

plugins/user/joomla/joomla.php

+	 *
+	 * @return  boolean
+	 *
+	 * @since  __DEPLOY__VERSION__


Align. Add one more space.

zero-24 · 2018-05-02T20:25:02Z

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 · 2018-05-03T17:07:07Z

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

Try to create another user with the same username. Error pops up. When you debug you notice that the $data array is empty

carlitorweb · 2018-05-04T19:32:50Z

I do not understand why the drone fail here?

zero-24 · 2018-05-04T22:11:30Z

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 · 2018-05-09T16:41:50Z

@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 · 2018-05-09T16:48:11Z

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

carlitorweb · 2018-05-09T17:16:20Z

Thank you @Quy

SharkyKZ · 2018-05-25T07:12:02Z

plugins/user/joomla/joomla.php

+	 *
+	 * @return  boolean
+	 *
+	 * @since   __DEPLOY__VERSION__


It's __DEPLOY_VERSION__ (one underscore between words).

SharkyKZ · 2018-05-25T07:18:44Z

plugins/user/joomla/joomla.php

+		if ($name === 'com_users.user')
+		{
+			// Passwords fields are required when mail to user is set to No
+			if (is_object($data) && $data->id === 0 && $this->params->get('mail_to_user', '1') === '0')


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 · 2018-05-25T12:20:12Z

Thank you @SharkyKZ

Quy · 2018-05-26T02:27:31Z

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 · 2018-06-07T20:42:33Z

Done, thank @Quy

plugins/user/joomla/joomla.php

mbabker · 2018-06-07T21:04:39Z

plugins/user/joomla/joomla.php

+		if ($name === 'com_users.user')
+		{
+			// In case there is a validation error (like duplicated user), $data is not a stdClass but just an empty array
+			// TODO: Check inside the FormModel line 230, for put the right associated data for the form


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

Is true, thank you @mbabker my mistake, sorry

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 · 2018-06-10T01:46:57Z

Related issue #17700 about empty $data.

carlitorweb · 2018-06-20T16:30:51Z

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

SharkyKZ · 2018-06-27T13:29:14Z

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 · 2018-06-27T15:07:32Z

Ok, let do the suggestion of @zero-24

brianteeman · 2018-06-27T15:29:45Z

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

mbabker · 2018-06-27T15:32:51Z

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 · 2018-06-27T15:39:37Z

This is a valid argument imo #20247 (comment)

ghost · 2019-07-21T06:48:32Z

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

carlitorweb · 2019-07-22T13:54:58Z

@fran so far, yes

ghost · 2019-07-23T09:01:49Z

"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 · 2019-09-02T16:23:23Z

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

SharkyKZ · 2019-09-02T16:24:22Z

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 · 2020-01-14T00:57:08Z

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 · 2020-01-14T00:57:17Z

RTC

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

HLeithner · 2020-01-22T21:38:22Z

Thanks

Fixed Hard Coupling (com_users | plg_users_joomla)

28edf5c

joomla-cms-bot added the PR-staging label May 1, 2018

ghost mentioned this pull request May 1, 2018

Hard coupling between com_users and plg_user_joomla #20249

Closed

zero-24 reviewed May 1, 2018

View reviewed changes

plugins/user/joomla/joomla.php Show resolved Hide resolved

zero-24 reviewed May 1, 2018

View reviewed changes

plugins/user/joomla/joomla.php Show resolved Hide resolved

Code review. Dropped instanceof JForm check

1c056ec

zero-24 reviewed May 1, 2018

View reviewed changes

Code review

ab35379

Quy reviewed May 1, 2018

View reviewed changes

plugins/user/joomla/joomla.php Outdated

*

* @return boolean

*

* @since __DEPLOY__VERSION__

Copy link
Copy Markdown

Contributor

Quy May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align. Add one more space.

Code review

239a5fd

carlitorweb mentioned this pull request May 2, 2018

Remove check for a valid form from two places in plugins #20277

Merged

Fixed issue when username is same

74b96eb

Try to create another user with the same username. Error pops up. When you debug you notice that the $data array is empty

carlitorweb added 2 commits May 4, 2018 15:37

Fix drone

7c05d6c

Fix drone

feec4ba

Merge branch 'staging' into hard_coupling_users

7ee9641

CS fixed

327ec06

SharkyKZ reviewed May 25, 2018

View reviewed changes

Codereview

3a5b6e3

ggppdk reviewed Jun 7, 2018

View reviewed changes

plugins/user/joomla/joomla.php Show resolved Hide resolved

mbabker reviewed Jun 7, 2018

View reviewed changes

carlitorweb added 2 commits June 7, 2018 17:56

Code review

ec4581c

Code review

fe03395

carlitorweb mentioned this pull request Jun 26, 2018

Erro create item menu #20879

Closed

ghost added the J3 Issue label Apr 5, 2019

ghost removed the J3 Issue label Apr 19, 2019

Quy added 2 commits December 5, 2019 15:03

Merge branch 'staging' into hard_coupling_users

d7a60f9

Merge branch 'staging' into hard_coupling_users

b633bbf

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 14, 2020

Merge branch 'staging' into hard_coupling_users

bf9ec93

HLeithner merged commit 474dc3c into joomla:staging Jan 22, 2020

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 22, 2020

HLeithner added this to the Joomla! 3.9.15 milestone Jan 22, 2020

Uh oh!

Conversation

carlitorweb commented May 1, 2018

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlitorweb commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zero-24 commented May 2, 2018

How to reproduce (First Issue)

How to reproduce (Second Issue)

How to solve the issues

Uh oh!

carlitorweb commented May 3, 2018

Uh oh!

carlitorweb commented May 4, 2018

Uh oh!

zero-24 commented May 4, 2018

Uh oh!

carlitorweb commented May 9, 2018

Uh oh!

Quy commented May 9, 2018

Uh oh!

carlitorweb commented May 9, 2018

Uh oh!

SharkyKZ May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlitorweb commented May 25, 2018

Uh oh!

Quy commented May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlitorweb commented Jun 7, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Quy commented Jun 10, 2018

Uh oh!

carlitorweb commented Jun 20, 2018

Uh oh!

SharkyKZ commented Jun 27, 2018

Uh oh!

carlitorweb commented Jun 27, 2018

Uh oh!

brianteeman commented Jun 27, 2018

Uh oh!

mbabker commented Jun 27, 2018

Uh oh!

carlitorweb commented Jun 27, 2018

Uh oh!

ghost commented Jul 21, 2019

Uh oh!

carlitorweb commented Jul 22, 2019

Uh oh!

ghost commented Jul 23, 2019

Uh oh!

SharkyKZ commented Sep 2, 2019

Uh oh!

SharkyKZ May 25, 2018 •

edited

Loading

Quy commented May 26, 2018 •

edited

Loading