Closed (fixed)
Project:
Web Experience Toolkit (7.x)
Version:
7.x-1.6
Component:
WetKit Core
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
6 Jun 2014 at 19:50 UTC
Updated:
18 Sep 2014 at 01:40 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
sylus commentedThis is rather unfortunate as I think media changed the permission name for this in the cleanup.
Not sure if this warrants another release. I'll try to see if can throw in a few other minor fixes to warrant one.
Soon with semantic versioning will be less of an issue.
Either way classifying this as a bug and thanks for reporting this :)
Comment #2
sylus commentedLooked at this a bit and yes a permissioned has been changed media side. Good news is this won't affect fresh installs at we simply give all administrator role permissions.
We will need to write an update hook to resolve this for existing installs. For now as mentioned above one can simply check the permission for "media browser".
Comment #3
sylus commentedComment #4
joseph.olstadEDIT* see comment number 19 with workaround/fix/patch
Comment #5
joseph.olstad*edit* see comment #19
Comment #6
joseph.olstadtrying to push a patch upstream. Here's the node where the original problem crept in.
Probably best to create a "new" issue for this, so I'll create a "new" issue upstream for this and reference it.
Comment #7
joseph.olstad*edit* see comment #19
Comment #8
joseph.olstadsee comment #19
Comment #9
joseph.olstad*edit* see comment #19
Comment #10
joseph.olstadok, I did some more testing, this is not an upstream issue. in our distro the hook never gets fired, most likely because we're using various versions of patched media modules (from previous RC's and V1.x's ) and it's likely that the update_7226 and update_7227 for that matter was already triggered by a different version that didn't actually update the permissions correctly. I tested by cleaning my db, reloading to a baseline and then changing the update_7226 to update_7228 , I'll have to check the schema table to verify, but it looks like our schema was at update_7227 , so the update_7226 must have been fired previously with a different patch and therefore never actually copied any permissions as it did something else with a previous patch code.
I'll close this again, it's on our distro side obviously
I'll check the schema table to see what our media schema is at after the v1.5 upgrade.
Comment #11
joseph.olstadI did a check on our roles in our system and
select rid, permission, module from role_permission where permission like '%create file%';resulted in:
select rid, name from role;
So there's no need to patch the media_update_7226 as it wouldn't do anything because our schema version is already at 7227 before v1.5 is installed, somewhere along the line another patch was run before this that already used update_7226 , and update_7227 for that matter , to track it down would have to look closer at the distro v1.4.
I also checked the "system" table and according to the "system" table the schema for 'media' after the upgrade to v1.5 is set to 7227.....
Before the upgrade when our site is at v1.3 it's at 7226, , but the permission is not set, so this is why we have no permissions. The hook media_update_7226 that "appears" in version v1.5 never gets fired because the schema is already at 7227 , probably because of something that happens in version 1.4 of the distro.
So to solve this problem, we would need to adjust the system value for 'media' and set schema value to 7225 in v1.6 so that media_update_7226 will run on the next updatedb.
Sylus, can you please take care of this? 30 comments and 3 issues later , this is the result ;P Do you want to change the schema value or just use my patch for wetkit_wysiwyg?
Comment #12
joseph.olstadComment #13
joseph.olstadMissing in this patch is some code to set the schema version of media back from 7227 to 7226.
Comment #14
sylus commentedThis is actually media + file entity not doing best practice as they reverted: #2104193: Default file entities are not exportable by features (Media File Entity Overridden) and the corresponding file_entity issue. When they reverted both these commits they also took out the hook_install_n invocation which you should never do when something has been committed to dev. Instead they should have left the hook but removed the code inside it.
The best fix is to set the schema for both file_entity and media to one back and let the original handling take out in the respective modules. I can get to this in a day or two.
Comment #15
joseph.olstadSounds good, so to confirm if I'm understanding correctly when you say
you'll set the 'media' schema back to 7225 and then on the next 'drush updatedb' the media_update_7226 will run?
Sounds like a good plan to me.
After seeing this I'm wondering if we should go back and compare current schema value with all modules.install hook values for all modules to locate the highest value and verify that the current schema value isn't greater than the highest value found in the module.install.
Might not hurt to go back a few versions checking this out. Might be easier to write some sort of code to do this sort of check automatically. Maybe there's some sort of verification test we could write (due to the nature of this maybe a custom module would be a good way to go about this, it might actually be quite simple to write).
This is not the first time this sort of thing occurred, I recall having to update the schema value for another module inbetween one of the RC's , except it was to skip an update for an alter table that couldn't run because the alter had already run previously. These are the sort of issues that can really bite back hard and it would be cool if we perform some sort of schema validation specifically for systems that are upgraded, it might be a good thing to have in general to validate various patching and changes made to various contrib modules.
Might have just cooked up ourselves a good use case to write a custom module if it already doesn't exist, a bit of an edge case but could be useful if a warning message was displayed when the 'drush updatedb' command was run and the validation code (comparing schema to module.install values) came up with something deemed unusual.
Comment #16
sylus commentedThis was an issue due to media removing a committed hook_install_update which is very rare from contrib side but file_entity and media move particularly fast. Yes it will just be a simple schema update to a version before and this overall issue pretty minimal in that it is just a permission.
There will be no value in checking other hooks as this was a one time occurrence and won't have occurred in other contrib. Additionally it is mitigated on fresh installs and easily fixed with a hook_update_n.
There will be many more important issues relevant to a dev effort primarily to address coming the upcoming bootstrap release.
I'll write a patch in a day or so to address this issue and we can proceed from there.
Comment #17
joseph.olstadautomatically fix schema / synch schema
Found some nifty example here and created a drush script (attached)
ran this script against our site upgraded to v1.5 (from v1.3)
Script usage:
chmod 750 sync_schema.dr
./sync_script.dr /path/to/drupal (OR run it from anywhere in drupal folder)
You can also comment out the line : drupal_set_installed_schema_version just to view to see if your schema is in sync, if it's in sync it won't find anything.
Comment #18
joseph.olstadthe .dr file didn't attach to the previous comment so I had to zip it.
see attached .zip
Comment #19
joseph.olstadSylus, this patch should resolve all the schema problems with media and file_entity and also corrects the permissions (reruns what should have been run in 7226 of the media update in v1.5 of the distro that never got ran).
So here's the fix.
Commit fix at your leasure. I think this is a good solution.
Comment #20
joseph.olstadSo to be clear, the patch in #19 runs the code found in update 7226 of the media update hook that was missed during the 1.5 upgrade
THEN it resets the schema values that are out of wack (media from 7228 to 7226 and file_entity from 7216 to 7215)
So before this patch: permissions are out of wack AND media and file_entity schemas are two and one ahead of actual.
AFTER the patch and a drush updatedb: permissions are corrected for access media module AND media and file_entity schemas are set to actual (correct) values.
Comment #21
joseph.olstadChanged the above patch, put it in wetkit_core instead , and make a resyncschema function so that it can be re-used more easily in case this problem re-occurs in the future.
Comment #22
joseph.olstadtrivial change to patch.
Comment #23
joseph.olstadpatch is for wetkit_core
Comment #24
gdaw commentedThank you Joseph for all your efforts to sort out the causes and fixes.
Comment #25
sylus commentedI have fixed this with the introduction of a new hook to wetkit_widgets and simply grabbing the logic from media + file entity.
The script mentioned above can still be used in development environments for tests but for now simply prefer calling the missing hook functionality directly.
Committed and attributed.
Comment #27
joseph.olstadReviewed the commits that went into v1.6 (wetkit_widgets instead of what I suggested in patch 22). Users that are only upgrading from v1.5 or are fresh installing won't have a problem but for the rest of us on older upgrade paths the system table schema being out of sync which actually is the root cause of the problem and may in the future cause further problems if left as-is. According to my extensive tests schema problems will occur for those that went through the 1.3, 1.4, 1.5, 1.6 upgrade path. Fresh installs won't have this problem as they won't have ever ran the messed up media_update_72XX against the schema.
The system table schema being out of sync occurs on sites upgraded from v1.3 or v1.4 to the v1.5 and v1.6 version of the distro. It will also affect sites running v1.2 and earlier versions that are upgraded to 1.3, 1.4, 1.5, 1.6.
Please add this function to a wetkit update hook which automatically fixes all schema issues and prevents the possibility of mixups in the future.
see:
wetkit_core_resyncschema();
as found in this patch
Perhaps I should have created a second issue for the schema out of sync issue but I thought I'd save time by providing a patch that solved all the issues. The wetkit_core_resynchschema() function needs to be called once during a hook_update on the next 1.7 version of the distro.
The wetkit_core_resyncschema() function will not hurt sites that don't have a schema out of sync problem other than taking a bit of cpu time to perform the hook_update. For those who've upgraded from < v1.4 it will find a schema out of sync on file_field and media. It logs what it does so if you're running a 'drush updatedb -y' then if your schema is out of sync it will tell you about it on the command line , otherwise the changes get logged to watchdog.
Next time to avoid confusion I'll split related issues into seperate issues. In this case there were two issues to deal with related to the problem.
*EDIT* 1) (still not fixed (see next comment)) the permissions that were not set due to the missed hook_update
2) (not yet fixed) the system schema for media and file_entity that was erroneously increased past current update hook values, basically a ratsnest of new problems waiting to happen if the schema isn't sync'ed properly to actual values as done with the function wetkit_core_resyncschema() as found in this patch.
Thanks!
Comment #28
joseph.olstadPatch 22 was tested to work in my environments, however the commit 2a2addf is different and according to our tests does not resolve the "media" permissions as expected/intended.
commit 2a2addf using wetkit_widgets commit b3d7c8c:
patch 22 (similar section of code, tested to work according to my tests (media permissions are correct after this is run)
The user_roles function returns the appropriate permissions in the patch 22 but not in commit 2a2addf /wetkit_widgets commit b3d7c8c
Please review comment 27 and 28 to:
A) fix the media permissions and
B) resolve the schema out of sync issue for those that upgraded from les than < v1.5 to v1.5 or v1.6.
Thanks
Comment #29
joseph.olstadif patch 22 was applied verbatim it would resolve the problem. However as the other commit was done to wetkit_widgets you could modify patch 22 to apply it to wetkit_widgets instead of wetkit_core.
Comment #30
joseph.olstadComment #31
joseph.olstadnot rc2 (oops) (see next comment)
Comment #32
joseph.olstadComment #33
joseph.olstadChanging priority to major as the schema out of sync problem can cause unpredictable results.
The two schemas that need re-syncing are: file_entity and media
See comment #28 and comment #29 for details about the schema sync and media module permissions.
Comment #34
joseph.olstadIf you apply patch 22 to wetkit_core this what you will see in the console output of a drush updatedb when upgrading from 1.4 or an earlier version of the distro(always one minor version at a time is how I'm upgrading). This resolves the schema out of sync for file_entity and media as well as the permissions that were missed in our upgrade path of the media module due to the mixup of the hook_installs from the dev branch upstream in media.
Result: fixes problem, prevents future issues with schema out of sync. and also runs missed permissions function that occured due to schema issues introduced between v.1.4 - v1.5 of the distro. The rest was picked up in 1.6 but this patch or a version of it should go into 1.7 and those wishing to upgrade to 2.x I recommend to upgrade all the way through to 1.7 before going to 2.x.
Comment #35
joseph.olstadWhile I do understand the risks of the schemasync function, particularly with regards to contrib modules, if we filtered it's checks to only wetkit_ modules then we could increase our control on our own wetkit_ modules. Regardless it is useful in debugging and and it did identify correctly when potential schema problems creep in.
file_entity
AND
media
see notes:
also, wetkit_widgets.install last schema update and the schema version found in the system table (7102) out of sync issue was probably because I ran the check after upgrading from 1.6 to 2.x and the 2.x branch of wetkit_widgets does not have the update code 7102 which was ran when updatedb was called after upgrading to 1.6.
I'm still curious why the TRUE value was changed to FALSE in commit 2a2addf using wetkit_widgets commit b3d7c8c:
patch 22 (similar section of code, tested to work according to my tests (media permissions are correct after this is run)
according to our tests (gdaw, ...) the permissions for media were still not set after running the new code. I suspect this might be due to the TRUE value being FALSE in wetkit_widgets_update_7102.
Comment #36
sylus commentedSo right now we only have file_entity out of sync which I have documented to watch out for once file_entity performs another schema update. I'll make sure to replicate the logic for that permission once it happens. Will also paste in the KNOWN_ISSUES document before next release.
The following code:
Should be false because if it was TRUE it would mean anonymous users would get permission to create files. You can see the commit itself in file_entity sets this to false at:
http://cgit.drupalcode.org/media/tree/media.install#n1168
All functionality now works with the patch so going to mark this as closed but will meet with gdaw tomorrow to confirm and reopen if he mentions any issues.
Thanks for your help on this one.