-
Notifications
You must be signed in to change notification settings - Fork 56
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
Simplify hook_civicrm_config, remove extension from include_path #269
base: master
Are you sure you want to change the base?
Conversation
Since totten#265 the `include_path` addition has not been needed except for odd edge-cases where the extension has stuff that's not covered by the autoloader. I'm honestly not sure what those things might be, but best-practice is to use the autoloader and not mess with `include_path`.
@totten this change doesn't require a |
I definitely like the idea of removing Why? Well...
If there's no one who wants to encourage the 1-by-1... then I'd be pretty inclined toward adding a generic |
@totten I like your mixin idea. Just to distinguish between our 2 suggestions:
From a code-organization POV a mixin does sound nicer and I'm generally +1 for that idea, however are we concerned about loading this during bootstrap? For reasons I still don't understand, it's been suggested (see #272) that hook functions are available earlier in the bootstrap process than mixins. Is that really the case and if so would that cause problems for anything that needs to be included early? |
Since #265 the
include_path
addition has not been needed except for odd edge-cases where the extension has stuff that's not covered by the autoloader. I'm honestly not sure what those things might be (maybe APIv3 files?), but best-practice is to use the autoloader and not mess withinclude_path
.