Currently, we include jquery_update in panopoly_admin.make and its .info file. However, all the jQuery code in Panopoly depends on jquery_update, and we are even including a jQuery UI theme in panopoly_core which is specific to the jQuery UI that we're getting from jquery_update.
I think it would make more sense if jquery_update were in panoploy_core.make and .info file since it's such an important dependency that pretty much all the Panopoly modules (but especially panopoly_admin, panopoly_core and panopoly_widgets) depend on.
Note: we probably can't actually remove it from panopoly_admin.make (for safe upgrades for users that may use panopoly_admin but not panopoly_core) but that's really an implementation detail. :-)
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | panopoly_core-move-jquery_update-2492811-6.patch | 3.1 KB | dsnopek |
| #6 | panopoly_admin-move-jquery_update-2492811-6.patch | 1.99 KB | dsnopek |
Comments
Comment #1
dsnopekEr, and added a note that we probably can't actually remove it from panopoly_admin.make (for safe upgrades for users that may use panopoly_admin but not panopoly_core) but that's really an implementation detail. :-)
Comment #2
robloachIsn't that the module that ties them all together? Seems strange to use panopoly_admin and not panopoly_core. It should be our one ring to rule them all. In any case, here's the patch to move it all, including the .make file. If they're using panopoly_admin and not panopoly_core, then they are probably smart enough to have jquery_update in their own .make file.
Comment #3
dsnopekThanks for the patches!
Probably, yes! But removing it from panopoly_admin.make breaks the upgrade guarantees/promises we're making for the stable Panopoly 1.x branch. We have an open discussion about how to handle removing things from .make files that was never fully resolved:
#2166865: Removing non-required modules from .info / .make files
What I'm proposing is that we move everything EXCEPT that we have to leave jquery_update in the panopoly_admin.make file, so it'll exist in BOTH the panopoly_core.make and panopoly_admin.make. This is only to keep the current upgrade promises we've made for Panopoly 1.x, so we should probably add a comment to both those .make files to remind us that this is why it's duplicated in two places.
If we can come up with either (a) a technical solution to removing stuff from .make files without breaking our current upgrade promises, or (b) a policy change to those upgrade promises that won't make people mad, we can definitely remove the duplicates later!
Anyway, returning to this issue - I'm marking as "Needs work" for:
Thanks again for your help on this! :-)
Comment #4
robloach2492811-panopoly_core-move_jquery_update_0.patch still applies. Uploaded the other media-related one to the other issue.
Do we need a note about this in Panopoly Core's .make? Seems like the only oddity is the one in Panopoly Admin .make.
Comment #5
dsnopekI think it'd be best to put a comment in both. That way when we're updating panopoly_core.make for newer jquery_update versions it'll be more likely that we'll remember to update panopoly_admin.make as well!
Also, could you update the panopoly_core patch to not include the CSS changes from #2235081: Update jquery_update to version 2.5? Let's keep these issues seperate so we can commit them independently.
Comment #6
dsnopekHere's updated versions of the patches!
The panopoly_core changes are the stuff I mentioned in the above comments and I added a
hook_update_N()for the case that the user may have used panopoly_core without panopoly_admin in the past.There's only a small change to the panopoly_admin patch: it keeps jquery_update as a dependency in its .info file (it still needs jquery_update) and keep the jquery_update version at 2.3 (we're working on updating that in a different issue).
EDIT: Here's a build on Travis-CI: https://travis-ci.org/panopoly/panopoly/builds/63697611 (mostly to test that the hook_update_N() works!)
Comment #8
dsnopekThe tests haven't finished yet, but the bits that matter have already run and were successful. So, committed!
@RobLoach: Thanks again for pushing this forward!