Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
laryn opened this issue Aug 29, 2024 · 7 comments · Fixed by #18 · May be fixed by #23
Open

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

laryn opened this issue Aug 29, 2024 · 7 comments · Fixed by #18 · May be fixed by #23

Comments

@laryn
Copy link
Member

laryn commented Aug 29, 2024

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:

@laryn
Copy link
Member Author

laryn commented Sep 14, 2024

@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
Copy link
Member Author

laryn commented Sep 14, 2024

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

jenlampton added a commit that referenced this issue Sep 18, 2024
Issue #17: Allow library via XAutoload+bundled OR Composer OR bundled.
@jenlampton
Copy link
Member

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.

@jenlampton jenlampton reopened this Sep 19, 2024
@jenlampton jenlampton changed the title Follow up to bundling library consider whether the library is already loaded before trying to use the bundled version Sep 19, 2024
@laryn
Copy link
Member Author

laryn commented Sep 19, 2024

@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
Copy link
Member Author

laryn commented Sep 21, 2024

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
Copy link
Member

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
Copy link
Member Author

laryn commented Oct 15, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants