drupal_valid_test_ua() only assigns a value to its static on a positive match.

This means that the entire function is re-executed under normal operations whenever invoked.

CommentFileSizeAuthor
#9 test_valid_ua_static-1411114-d7-9.patch295 bytesalbert volkman
drupal8.test-valid-ua-static.0.patch315 bytessun

Comments

bleen’s picture

bleen commented

This is a no-brainer ... patch applies fine.

As soon as testbot says ok, RTBC

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

RTBC it is.

David_Rothstein’s picture

David_Rothstein commented

This seems harmless, but the only thing it really bypasses in that case is a single call to preg_match()... Is it really worth statically caching that?

dries’s picture

dries commented

I don't think it is worth caching, actually.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

In case of a positive preg_match(), further processing and calculations are executed. And those should be cached.

This is a mere Boolean static value, which has no weight at all in terms of memory.

David_Rothstein’s picture

David_Rothstein commented

But a positive preg_match() only occurs for a valid simpletest request (which is already cached) or for someone trying to hack the site by pretending to be simpletest (which is pretty unlikely)... right?

That said, it definitely seems harmless in terms of memory usage.

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I've gone ahead and committed this, since we're not adding any new static caching here, just fixing the existing one.

I'm not sure it's worth static caching this, so I opened #1436684: Remove static cache in drupal_valid_test_ua(), however there are an increasing number of patches in the queue trying to check this for bizarre simpletest edge cases, so I'd not want to remove it without discussing a bit more.

albert volkman’s picture

albert volkman commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new test_valid_ua_static-1411114-d7-9.patch295 bytes

Easy D7 backport :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.