Improve tests for user form alters in contact module

Thank you, marcingy.

I faced this warning, and the patch you provided, made its job.

patch works

Ouch. Looking at the code I think this should call $account->isAuthenticated() instead of !user_is_anonoymous(), because the function checks the global user, but we're really interested in the user that we are changing here. We want to know if he has an ID that we can look up. (We could also do $account->id() > 0 to make it more explicit, but that's exactly what isAuthenticated() does).

Trying with $account->isAuthenticated(), if it does the job.

Yes, this does make sense to me, but we lost the test coverage from the previous patch now. Make sure to provide a version of the patch that contains just the test change and one with the test change and the fix.

Fixing component.

Attached is the test which should fail when creating new users and contact module is active.

Change of status

Thanks, we also need a combined patch with fix and test that is green and can be committed :)

I couldn't recreate the error with PHP 5.3 (tested i with the testing framework with UserCreate within drupal 8), so I asumme this error only applies for PHP 5.4.

Here is the fix that includes the test as well.

Right, those kind of notices are PHP 5.4 specific.

I confirmed that the test in #11 throws notices for me. Can we add a comment to the docblock above $modules and explain why contact is enabled here? (Something like Enable contact to make sure that it correctly handles user create forms.)

Just a combined patch is fine.

Here is the combined patch with the docblock comment.

Change status.

Can anyone confirm that the patch 15/17 works as expected.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateTest.php
@@ -16,10 +16,11 @@ class UserCreateTest extends WebTestBase {
    * Modules to enable.
+   * Enable contact to make sure that it correctly handles user create form.

Sorry, per coding standards, there needs to be an empty line between the first line of a docblock and additional explanation.

And more importantly, the patch only contains the test and isn't actually a combined one.

Sorry about that. This one is the combined patch.

I was tried to apply the patch, but it failed, so rerolled it, also made some fixes:

+++ b/core/modules/contact/contact.module
@@ -324,7 +324,7 @@ function contact_form_user_form_alter(&$form, &$form_state) {
+  $account_data = $account->isAuthenticated() ? Drupal::service('user.data')->get('contact', $account->id(), 'enabled') : NULL;

Use '\Drupal::serviece' instead of 'Drupal::service'.

Also removed the line ending space from the doc block in the test.

I can also confirm that the test throws exceptions without the fix.

#22: array-to-string-conversion-2030743-23.patch queued for re-testing.

I can confirm that the patch works as expected.

Sorry to set this needs work yet again for a one-line fix, but...

Since this is contact module's functionality, it really ought to be tested by a test in contact module, not user module.

There's already a testPersonalContactAccess() which I think just needs to be extended a bit to cover the admin/people/create use case.

Tagging so I can find this in the RTBC queue later.

Tagging for reroll.

This still applies to 8-x HEAD and does not need reroll at this time.

Going to see if I can make webchick's requested fix.

Actually I'm going to hands-off on this on the basis that I don't know how to write tests and this would be my first code contribution to Drupal. I'm going to continue to work on it, and if I come up with a solution and it hasn't been solved by then, I'll go ahead and supply that solution. In the meantime, I don't want to hold things up if someone else can take care of this more easily than I.

Learned how to write tests.. taking this.

First Drupal core contribution.. first Drupal code contribution, actually :D

1. +++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
   @@ -102,6 +102,10 @@ function testPersonalContactAccess() {
   +    ¶
   

   Some trailing whitespace here.

2. +++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
   @@ -102,6 +102,10 @@ function testPersonalContactAccess() {
   +    // Test that 'Personal contact form' checkbox exists
   

   Missing a . at the end.

When you upload the next patch, and you also upload one of just the test by itself, without the fix? That will ideally fail, proving that its actually testing it.

Will do!

Ok, so bit confused here: the test I wrote is to validate that the personal contact form checkbox on admin/people/create shows up - but this "Notice" error didn't inhibit that functionality to begin with. What test is expected here? Should I just remove the test and supply that patch that removes the notice error?

Ok I read back a bit and I think I might understand this now.. creating a user with contact module active would fail? I will test this and write an appropriate test for it if I can confirm the behavior.

So, I've tested on 8.x HEAD and I don't experience any functional errors related to this error. I am able to successfully create users with the contact module enabled, both with and without the personal contact form checkbox checked, and the value is properly preserved. I am entirely unclear on the purpose of testing in this issue, unless we are testing for the absence of this error message, which wasn't my impression. Not sure what to do at this point.

There was just one line that had unneeded whitespace so I got rid of that. Everything else looks good to me.

38: 2030743-38.patch queued for re-testing.

32: array-to-string-conversion-2030743-32.patch queued for re-testing.

Hmm. It looks like removing the whitespace causes a test failure, so I'll give a +1 to the patch at #32.

This is still broken on admin/user/create, and it looks like #32 is super close to an RTBC once #33 is taken care of. Any takers?

I will figure it out, since it was my patch.

Clarification on what I need to do:
- Reroll.
- Ensure the User module contains a user creation test that loads the form (presumably it auto-tests for PHP errors). Testbot isn't catching this issue because it doesn't happen on PHP 5.3.
- Correct the formatting issue (period) that DOESN'T cause tests to fail.. or maybe try correcting the space issue too.

* Reroll not needed, but I just re-implemented the 1-line patch from scratch, so I guess that counts as a reroll.
* Confirmed test coverage in UserCreateTest.php line 78: $this->drupalGet('admin/people/create');
* Without the test, there's no longer a need to fix any whitespace or formatting issues.

*crosses fingers* This has been a heck of a confusing first-time-contributor issue.

Am I missing a step to make testbot test this?

Yes, you need to set it to needs review, but it doesn't matter right now, because we still need a test that lives in contact module. Not sure if I understood your comments correctly, are you saying that you can't reproduce the error anymore?

I'd expect that the error still exists, and if that's the case, then we need a test for it. We cannot commit just a the fix.

Berdir: There is no functional test for this issue, as far as I can tell. The error doesn't affect functionality. I spoke with webchick, and she indicated that we don't write tests purely to test for error messages?

If there's a test to write here, I need some REALLY explicit guidance on what I'm supposed to be testing for.. because it's been clear as mud up until this point :)

A notice is a bug, bugs need tests. Would be new to me that we don't do that :)

Also, If I'm not mistaken, then this is a functional bug. The checkbox default for new users should be enabled, but if you create a new user as admin, the checkbox isn't checked. And there are more hidden bugs here, if you save then, there is no value saved in the users_data table and it falls back to the default. The reason for this that $account in the submit callback doesn't have an ID, so it doesn't go into the if () there. That might be a bit trickier to fix.

Anyway, you see that there's a lot of stuff to write tests for :)

I already wrote a test for editing an existing user's form at the end of ContactPersonalTest::testPersonalContactAccess(). So, we have a bunch of scenarios to test there:

1) Create an admin user with administer users permission.
2). Create a new user at admin/people/create. Make sure the checkbox is checked by default (assertFieldChecked())
3). Then submit with the necessary values (username, email, password). Then load the user based on the username, go to user/ID/contact, make sure that you don't get an access denied and e.g. see the page title.
4) Create another user, explicitly disable the checkbox (submit FALSE as value to drupalPostForm()), then load the user again, visit user/ID/contact and assert that the response is now 403 (assertResponse()).
5) Change the default setting (\Drupal::config('contact.settings')->get('user_default_enabled')) to false, then create yet another user make sure the checkbox is now not checked by default (assertNoFieldChecked()), submit without changing the default value, visit contact page again, should still be 403.

Then, applying the patch here should fix 2), but it will not fix 4) and 5). We need to look into how to do that. Removing novice and Quick Fix tags, as this might be neither, but please work on the test :)

I didn't realize the checkbox was supposed to be checked by default, so yeah, that changes everything. This is (well) outside the realm of my testing knowledge, so I'm going to release this one back to the wild, as I don't have immediate plans to expand my knowledge of SimpleTest.

Still get this warning when using php 5.5.3

Notice: Array to string conversion in form_process_checkbox() (line 1299 of core/includes/form.inc).

I spotted the issue (the PHP notice) still here this week.
The actual issue has been pulled off-topic into providing functional tests for things that needed tests anyway but were not directly related to the bug itself.
The tests are good steps forward though. I needed a slight re-roll to deal with a conflict/fix that had already gone in in the meantime.

To address the reported issue:
What we are getting is the "String to array conversion" notice, which is due to the default value of a user.data value returning array() instead of NULL or the default in cases where there is no user yet. Such as when a user is being created for the first time.

\Drupal\user\UserData::get() seems to be introducing this array when our bool was expected.

  $account_data = \Drupal::service('user.data')->get('contact', $account->id(), 'enabled');

Using \Drupal\user\UserDataInterface::get() with the uid parameter set to NULL is wrong here, as that is asking the API to return a list of user.data contact flags fields for all users that met the criteria (hence the array).

We actually want the result for a single user, so the uid argument must be set to something. It's just that the user doesn't yet exist,

I found that by setting the uid to 0 instead of NULL for the new user, a single (NULL) value for the user.data contact flag was correctly returned. I couldn't spot any other place to define "This is a new uninitialized user so don't actually do any data lookup for it".

  $account_data = \Drupal::service('user.data')->get('contact', (int)$account->id(), 'enabled');

With this approach, the current error goes away.

Alternatives to this include:
* Adding new logic here in the form to detect if it's a new user and dealing with that as a special case. I don't favor that as I think we have uncovered a deeper issue that is "What should UserDataInterface::get() really be doing when fetching data for a new uninitialized user entity?"
* Using uid 0 in place of NULL for this API call. Which is what I'm doing, though I feel it incorrectly merges the idea of "new" user with "anon" user.
* Using something like -1 to indicate "Get me data for a single user, a user that does not exist". I'm not doing that as I think it introduces a new trick.

So this patch is a small re-roll of #58, and includes the fix for the PHP notice here.
Also a minor fixup - the value for the flag us a bool in most other places in the code, but was an int in one place. Switched the '1' with 'TRUE'.

Nice, been looking for this issue. This has been annoying me :)

Nice detailed write up, I can confirm the root of the problem from debugging too.

How about this approach instead? We can add a check is isNew() in the ternary conditional instead?

1. +++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
   @@ -210,6 +210,55 @@ function testPersonalContactFlood() {
   +    $config = \Drupal::config('contact.settings');
   +    $config->set('user_default_enabled', FALSE);
   +    $config->save();
   

   We could just chain these?

2. +++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
   @@ -210,6 +210,55 @@ function testPersonalContactFlood() {
   +    $this->checkContactAccess(403);
   

   We could test again with the user_default_enabled set to TRUE? or test that first actually. As if the array_to_string thing is happening the checkbox will be empty when it should be checked.

I'd be happy with isNew() also (didn't know the API well enough to go looking for it)
That's explicit about what it's doing, and means that the 'special case' is now trivial.

It only leaves open the question about what happens in other places if a user is created via code elsewhere - do they have this problem also? But that's not todays problem.

Looks the actual fix got in with the same test change that was pushed back here for not being proper enough in #2231495: Array to string conversion error when trying to add a new user.

Rescoping this one to add the tests we have here instead.

63: 2030743-63.patch queued for re-testing.

Rerolling patch in #63...

69: 2030743-69.patch queued for re-testing.

webchick in #26 here asked to not add contact to the user test, which was done in #2231495: Array to string conversion error when trying to add a new user. taking out that since we add the actual test coverage here.
and we dont need the "fix" in contact.module since it was fixed in 2231495 and the id() is the same as the !isNew()

Thanks, looks good.

+1 RTBC

Straight re-roll

Committed 1a53cd1 and pushed to 8.x. Thanks!