consider whether the library is already loaded before trying to use the bundled version

[laryn]

I'd like to make this module maximally flexible for how people want to provide the library -- right now it bundles the library and has a hard requirement on XAutoload. On a few other modules I've begun to provide flexibility by considering whether the library is already loaded by some other method before trying to use the bundled version of the library. This makes XAutoload a soft requirement. Would you be open to something like that? Here's a PR that's near completion for another module as an example of what I mean:

• https://github.com/backdrop-contrib/sendgrid_integration/pull/9/files

[laryn]

@yorkshire-pudding I see you have previous commits on this module -- are you interested in testing this one? Even if you don't test loading the library with Composer, I think testing with or without XAutoload would be good.

[laryn]

I've tested this PR with Composer Manager installed and that seems to work nicely.

[jenlampton]

I merged the PR since it sounded like a good idea, but now that I've been using the module for a bit I'm having some trouble.

Reopening the issue because calling class_exists() during hook_autload_info() is likely to cause all sorts of problems. class_exists() will call hook_autload_info() to find out what classes exist... so we're likely to end up stuck in loops.

I'm actually encountering all sorts of problems already, just trying to do a requirements check for the status report, so I'm going to rework this to pull out the class_exists() check.

@quicksketch recommends using hook_autoload_info_alter() instead. @laryn I'm not sure if you want to try this again using the alter?

This makes XAutoload a soft requirement.

By bundling the library with the module, we wouldn't be using Xautoload (or libraries) anymore at all, since the module will load the library on its own.

[laryn]

@jenlampton That makes a lot of sense regarding class_exists inside hook_autoload. Oops! In this library, since it doesn't have a bunch of other dependencies that need to get downloaded along with the library (and the library basically functions on its own) I don't have strong feelings about allowing it to be loaded externally with Composer. It's mainly when a library requires a bunch of other libraries that relying on bundling gets hairy. What's your preference? I can revise and try to work this into hook_autoload_info_alter() or just strip out the composer.json.

[laryn]

Another thought: class_exists has a second parameter that defaults to true, to autoload the class in question. We could simply set that to false if this function was used inside hook_autoload_info?

[jenlampton]

My preference is not to have varrying results from hook_autoload_info(), but if you can think of a way to do it in hook_autoload_info_alter() I'd be open to that.

[laryn]

@jenlampton It's not clear to me how it's better to set it and then unset it in a follow-up alter -- it seems less efficient and makes more code to maintain. Is there a reason or is it just personal preference?

I'm testing locally with the if (!class_exists('Flow\JSONPath\JSONPath', FALSE)) { section within the hook_autoload_info() and it works in the following scenarios:

• No other library-loading modules enabled ✅ uses bundled library
• Composer Manager installed, but have not yet run composer update to load the library into the vendor directory ✅ uses bundled library
• Composer Manager installed, have run composer update and loaded library into vendor directory ✅ uses library from vendor directory

If you still strongly prefer the other, I'll try to implement it in hook_autoload_info_alter() by unsetting the library-specific values that are set in hook_autoload_info.