Currently get_extension_funcs is not supported in HHVM. It is only used in image_gd_check_settings(). Will create a patch to use function_exists instead of get_extension_funcs.

CommentFileSizeAuthor
#18 drupal-2161955-18-gd-remove-get_extension_func-D7.patch617 bytesmikeytown2
#13 gd-2161955.patch1.61 KBdawehner
#10 2161955-remove-get_extension_func-10.patch1.54 KBwiifm
#9 2161955-remove-get_extension_func-9.patch1.54 KBwiifm
#1 drupal-2161955-1-dont-use-get_extension_funcs.patch629 bytesmikeytown2
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented
Status: Active » Needs review
FileSize
drupal-2161955-1-dont-use-get_extension_funcs.patch629 bytes
Garrett Albright’s picture

Garrett Albright CreditAttribution: Garrett Albright commented

Huh. What benefit does using get_extension_funcs() have over simply function_exists() in the first place? Aside from HHVM compatibility (discussion thread here), it seems like far more straightforward code.

mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented
Status: Needs review » Reviewed & tested by the community

I'll go ahead and RTBC this as it cleans up the code making core more readable.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

+1 to commit that

mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented

Side note via gotta_download_them_all; these are the module that use get_extension_funcs()
grep -l -r -i "get_extension_funcs(" ./

./allmodules/acidfree/image.imagick.inc
./allmodules/dragdrop_gallery/ImageResize.php
./allmodules/dcco/modules/system/image.gd.inc

https://drupal.org/project/acidfree (D6)
https://drupal.org/project/dragdrop_gallery (unpublished due to SA)
https://drupal.org/project/dcco (core is inside so this is a non issue)

So core it the only place that needs the fix from what I can see. Also noted that I had to fix one of my modules in order to be HHVM compatible #2162025: Warning: Invalid argument supplied for foreach() in httprl_send_request() line 1528 of httprl.module.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I'll go ahead and RTBC this as it cleans up the code making core more readable.

just don't do that.

mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented

just don't do that.

I find that I need to put things to RTBC for smaller simpler patches in order to get guidance on it a lot of times. This patch waited over a year before it was tagged as a duplicate #1633130: Path admin/structure/menu/parents returns 403 unless UID=1.

Patches that are small usually don't require much review as well #1937860: seven_css_alter does not set the type. I got a this looks good from #2 so I moved it to RTBC.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

Seems reasonable (git blame for this goes all the way back to #18700: image.gd.inc isn't compatible with gd 1 which doesn't seem to have a clear reason for not using function_exists())...

But this code is in Drupal 8 too. (And there's a second call to get_extension_funcs() there also.)

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
CreditAttribution: wiifm commented
Status: Needs work » Needs review
FileSize
2161955-remove-get_extension_func-9.patch1.54 KB

Re-rolled against D8 HEAD, using the same syntax as in #1.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
CreditAttribution: wiifm commented
FileSize
2161955-remove-get_extension_func-10.patch1.54 KB

Spotted an awkward double space in the code, new patch with correct spacing attached.

The last submitted patch, 9: 2161955-remove-get_extension_func-9.patch, failed testing.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
CreditAttribution: wiifm commented

heh, one patch passed an the other failed. Both are functionally equivalent. Test bot sad face. Can anyone RTBC this?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
gd-2161955.patch1.61 KB

Can't we just do the following?

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

This is definitely the cleanest one. RTBC if green.

Does anyone have idea why get_extension_funcs() was used in the first place?

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs review » Reviewed & tested by the community
mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented

#13 Looks good. +1 for RTBC

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 8.x-dev » 7.x-dev
Category: Support request » Task
Status: Reviewed & tested by the community » Patch (to be ported)

Nice!

Committed and pushed to 8.x. Thanks!

Marking back to 7.x.

mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented
Status: Patch (to be ported) » Needs review
FileSize
drupal-2161955-18-gd-remove-get_extension_func-D7.patch617 bytes
mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented

Patch passes tests. +1 from me for RTBC.

mimes’s picture

mimes CreditAttribution: mimes commented

Patch in #18 complies with Drupal coding standards and appears to fix the stated bug.
It's worth noting that there's an issue recommending the depreciation of function_exists() for is_callable() for Drupal 8 and another here, but it's probably too late to do so now.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

That's not really relevant here, as we're checking an actual, hardcoded function.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: HHVM compatibility » Remove get_extension_funcs() call to improve HHVM compatibility
Parent issue: » #2165377: [meta] HHVM compatibility

Created meta issue for HHVM: #2165377: [meta] HHVM compatibility

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented

#14:

Does anyone have idea why get_extension_funcs() was used in the first place?

There was a discussion in #1069140: Requirements should be provided by image toolkit, comments #40 to #42. I obviously (as can be deducted from those comments) agree with not using get_extension_funcs().

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: drupal-2161955-18-gd-remove-get_extension_func-D7.patch, failed testing.

mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented
Status: Needs work » Needs review

18: drupal-2161955-18-gd-remove-get_extension_func-D7.patch queued for re-testing.

Test went fatal on something unrelated; retesting.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
Status: Needs review » Reviewed & tested by the community

Tests succeeded.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: drupal-2161955-18-gd-remove-get_extension_func-D7.patch, failed testing.

mikeytown2’s picture

mikeytown2 CreditAttribution: mikeytown2 commented
Status: Needs work » Needs review

18: drupal-2161955-18-gd-remove-get_extension_func-D7.patch queued for re-testing.

Test went fatal due to several missing tables.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
Status: Needs review » Reviewed & tested by the community

Tests succeeded.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

And thanks for the explanation of why get_extension_funcs() might have been used originally. I agree that better support for HHVM is more important than the edge case where someone defined imagegd2() without the intention of it replacing the one from the GD library (that is really not something anyone should be doing anyway).

Status: Fixed » Closed (fixed)

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