This patch converts the user module please look over it closely for mistakes or misinterpretations i might have made concerning the point of this antipattern resolution and ill do the rest this week.

This patch:
* replaces a number pf plain statics with the new function based approach for consistency
* Removes the $reset argument from the user load functions and moves the reset_cache operation under the user save operation.
* Removes the $reset argument from user_access which called user_role_permissions to support dynamic roles. This was otherwise not called in core to actually reset but we will need contrib module upgrade notes for modules that want to support dynamic roles.

Questions:
drupal_cache_reset is called twice under user_save() but could probably be called only once above the "if" assuming we are ok invalidating the cache this far into a user save, i didnt want to move it as there might be an consequence i dont forsee.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

michaelfavia’s picture

FileSize
7.1 KB

updated patch to use -up. anyone know how to make eclipse show function names in patches?

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

Status: Needs work » Needs review
FileSize
8.76 KB

Two wrapper functions, user_load_reset() and user_access_reset(), were created to reset the static vars, as per #422362-20: convert form.inc to use new static caching API.

Upgrade notes should be written somewhere to track the removal of these $reset params. I don't know where that would go though.. ^^"

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

Status: Needs work » Needs review
FileSize
14.48 KB

Silly me. I missed all the user.test changes. ^^"

JamesAn’s picture

FileSize
14.48 KB

Ugh. Forgot a semi-colon. -_-

JamesAn’s picture

FileSize
14.48 KB

Lol. Uploaded the wrong patch.. -_-"

JamesAn’s picture

FileSize
14.79 KB

Just can't seem to get away from this issue!

I found a typo. Not my fault this time! XP
'User admininstration' corrected to 'User administration'

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

Status: Needs work » Needs review
FileSize
14.57 KB

Rerolled against #486274: Typo in user.test, which fixed typos.

The last submitted patch failed testing.

JamesAn’s picture

FileSize
14.49 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

Status: Needs work » Needs review
FileSize
1.12 KB

Rerolled. A lot of the changes are no longer necessary as user_load uses entity_load that doesn't use static vars.

catch’s picture

Status: Needs review » Needs work

When a single function has two statics, we should have a single $cache = &drupal_static(array());

then $cache['accounts'] and $cache['categories']

This reduces total calls to drupal_static() during the request, which can be a performance hit if a function using it gets called more than a couple of times (which anything using a static variable should be.

Pasqualle’s picture

Status: Needs work » Needs review

#16: 423806-16.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 423806-16.patch, failed testing.