The documentation of hook_image_toolkits() is inaccurate.
http://cgit.drupalcode.org/drupal/tree/modules/system/system.api.php?h=7...

The documentation says:

* The file which includes each toolkit's functions must be declared as part of
* the files array in the module .info file so that the registry will find and
* parse it.

Since the #497118: Remove the registry (for functions) is fixed the functions are not part of the registry. Instead this should say The file which includes each toolkit's functions must be included in this hook. See https://api.drupal.org/api/drupal/modules%21system%21system.module/funct... for an example.

Comments

chx’s picture

Issue summary: View changes
Status: Needs work » Active
Issue tags: -low hanging fruit +Novice

There's no patch so the issue status is "active" also the tag used for these is "novice".

pushpinderchauhan’s picture

Status: Active » Needs review
StatusFileSize
new654 bytes

Please review patch.

sweetchuck’s picture

There are some issue in the core's image toolkit system.

The problem is that when you need to do something with the image tookit you have to know which one is the current.
Call the image_get_toolkit() which calls the image_get_available_toolkits() which collects the available toolkits by the hook_image_toolkits() at this point all image toolkit file/function will be included even if it is not active.

The problem is that all image toolkit file/function have to be included because there is a very silly fallback system in the image_get_toolkit().
If the configured image toolkit is not available then it uses the first available toolkit.
This fallback system is nowhere promoted on the UI. I think nobody knows it is even exists.
The image tookits are ORDER BY
- hook_module_implements_alter()
- {system}.weight
- {system}.name
The site administrator has no control over the fallback system.

It would have better to add weight, path and file properties to hook_image_toolkits() and then the core would have been able to include only the necessary files.

Yes, you can include the necessary files in the hook_image_toolkits(), just because the result of the module_invoke_all('image_toolkits'); is not cached with cache_set().
But very ugly. And maybe there is no better solution.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

RE #3, are you saying that the patch is not good? Is all of this commentary a separate issue?

Because I think the patch looks fine for the issue originally reported... and I'm not following all of #3 but I think it's a separate issue (and maybe not just documentation)?

sweetchuck’s picture

The last paragraph of comment #3 is my relevant opinion about patch #2
I just tried to explain what is the problem with the toolkit system.

patch #2 RTBC++

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed to 7.x.

  • jhodgdon committed d84d4ce on 7.x
    Issue #2383491 by er.pushpinderrana, Sweetchuck: Inaccurate...

Status: Fixed » Closed (fixed)

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