Updated: Comment #20
Original Problem/Motivation
I *finally* had a chance to play with the D7 port of this module in preparation for #1779280: Create D7 release of Update status advanced settings. Although it's basically working, and all the D6-esque functionality is fine, there's a pretty major bug in the interaction between this and the new Update manager functionality in core. When you set a project to "ignored", it's not being ignored by the update manager itself. Although the alter hook is firing and we know the update "status" of the given project is now UPDATE_NOT_CHECKED, update manager doesn't correctly handle this case. :/ That's a core bug, but obviously we can't deal with that here. I'll open a separate issue for core.
Proposed resolution
Until core is patched and a new release is available, I see a few options (we could do more than one of these):
A) Document that people using update_advanced should probably *not* be using update_manager (at least in the README.txt and project pages)
B) Perhaps complain via the UI (e.g. a requirements check that if update_advanced is enabled and the update manager killswitch is still enabled, issue a warning).
C) More closely investigate if there's a work-around or some way via update_advanced code we can get this working.
Remaining tasks
Implement patch comment #16 for D8
Commit patch comment #16to D7 core - > 7.24
Comments
Comment #1
hass commentedupdate_manager means update.module? Can you share the core issue ID, please? If this is a core bug, I'm not sure why this needs to hold a release back. The major impact is currently not clear to me. Is any data going to be lost?
Comment #2
dwwUpdate manager is the part of update.module in D7 core that lets you install new modules and automatically upgrade all the modules you've got installed that are missing updates. The major impact is that if you ignore some problematic project or release via update_advanced to keep the available updates report sane, if you then invoke the update manager, those problematic projects/releases will be upgraded anyway, not ignored. It's not about losing data, it's about upgrading code you didn't want to upgrade.
The mitigating factor is that people who want/use update manager to upgrade their site probably aren't the target audience for update_advanced. But, currently, update_advanced *only* impacts the listing page at /admin/reports/updates (the default "List" tab) not the update manager itself at admin/reports/updates/update (the "Update" tab).
There's no core bug issue ID yet since I wasn't able to look more closely at exactly if/what core is doing wrong and write it up.
As I said, at the very least this issue blocks a release of update_manager so we can document that these two things don't work together yet. Ideally, we'd fix it, but perhaps we need to fix core for that.
Fun! ;)
-Derek
Comment #3
hass commentedAh, ok... got it now :-). I've seen this issue myself, but I had no time to look into it, too. But again if this is a core bug it should not hold a release of the module back as it's not the source of the issue.
Can we make a RC1?
Comment #4
dwwAgain, at the bare minimum, I want to document this better before releasing.
Also, I'm not 100% sure there's nothing we can do in contrib to get around this problem. Maybe we just need to use a different reason code when altering the status and it would all work as expected (more or less).
Yes, an RC1 would be okay, once README.txt at least mentions this limitation/bug/incompatibility... I'll work on that later today, inshallah.
Cheers,
-Derek
Comment #5
dwwOkay, pushed a fix to the README.txt about this:
http://drupalcode.org/project/update_advanced.git/commit/0098043
I still wouldn't mind investigating this more closely before RC1, but I also know most people using this module don't care about or use Update Manager...
Comment #6
klonosStill, did you file any issues against core so we can fix this in a proper way at some time in the future?
Comment #7
dwwAgain, I'm still not sure it's actually a core bug, so I'm not going to create an issue until I better understand what exactly is going wrong. If someone else wants to, I'm not going to stop you. However, I invite interested parties to first look closely at the core update manager code and see why it's not handling this case properly, and if there are other cases it handles that would be more appropriate for update_advanced to use for its needs.
Thanks,
-Derek
Comment #8
hass commentedI think we need to hook into
update_get_projects()wheredrupal_alter('update_projects', $projects);is used and remove the project that we ignore from the$projectarray. Than the project is not checked on d.o.. Update Advanced does not implement ahook_update_projectsyet...This is all code wise... have no dev env with me. Just like to post my findings that it is more a bug of update_advanced than core.
Comment #9
dwwCool, thanks for looking. My only fear of using hook_update_projects_alter() is that we want to make sure we don't completely remove the projects from the regular report case -- we just want them to be ignored by the update manager parts. But yeah, I think this might have been the use case I had in mind when I added that alter hook. ;) Let's hope it's sufficient and we can fix this before 7.x-1.0.
Cheers,
-Derek
Comment #10
hass commentedComment #11
hass commentedNot really. I had some troubles while digging into this. If we remove the ignored project with
hook_update_projects_alter()it's also removed from the list inadmin/reports/updates/settings. This is not what we need.I'm attaching a core update module patch as I have not found any other solution and moving the case to core for now. Please review.
Comment #12
hass commentedJust one screenshot how this looks like:
Comment #13
hass commentedAlso see #1963458: Caching does not flush/refresh project data properly, please.
Comment #14
hass commentedI'm attaching a new patch that replaces the tableselect with a themed table. This will remove one useless column and looks smarter. Also renamed Extra to Other to make sure we have no string changes.
Comment #16
hass commentedDamn windows LFs
Comment #17
dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #18
Anonymous (not verified) commentedReviewed:
I made a functional test on my dev server with patch #16.
Results are good.
Ignored versions are not present in the update list, waiting for a new version. When a new version is found on drupal.org the setting is changed to 'always'. I don't know if the note field in settings is emptied..?
Setting 'always' is also working ok, they are ignored in the update list.
Comment #19
David_Rothstein commentedThis looks like it needs to go in Drupal 8 too?
Comment #20
Anonymous (not verified) commentedThe patch works perfect on my dev server for a few months now.
Please, make this part of D7 core - > 7.24! (missing this in 7.23)
And to D8.
Comment #20.0
Cyclodex commentedupdated issue summary
Comment #21
Cyclodex commentedtried to updated the issue summary using the template
please verify and change it if its not 100% correct thanks
Comment #21.0
Cyclodex commentedadding task to commit patch
Comment #22
xjm16: update_Projects+ignored+by+update_advanced+are+still+processed2-D7.patch queued for re-testing.
Comment #26
hass commentedComment #30
mattshoafI triaged this at DrupalCon Baltimore2017 I'm changing this to Normal priority since it was brought up for a contrib module that is not being ported to 8, see: Update Status Advanced Settings. It would take more testing to determine if this affects 8.x at all. I'll leave it to the maintainers for if they want to change the version back to D7 only, or perhaps the status to Closed (Won't Fix) would be applicable as well.
Comment #31
mattshoafComment #37
izmeez commentedPatch in #16 still applies without difficulty to drupal 7.78
Comment #38
izmeez commentedUpdated title to better reflect this is a core bug that has a D7 patch in comment #16 but no movement on D8/9 despite being "Triaged D8 major" although may only impact the contrib module https://www.drupal.org/project/update_advanced.
Comment #39
izmeez commentedAdded this issue to #3192080: [meta] Priorities for 2021-04-07 release of Drupal 7 to improve visiblilty.
Comment #40
izmeez commentedThere was never any activity on this issue for Drupal 8 probably because it may only affect the update_advanced module with no plans for a D8 version. I am going to change settings to drupal 7 and see if the patch in comment #16 passes tests.
Comment #41
izmeez commentedThe patch in #16 passes and we have used it for some time. Changing status to RTBC. Also removing the D8 tag as it is irrelevant.
Comment #42
poker10 commentedUnfortunatelly I think that it is too late for such significant UI change in D7. The patch creates a new group in the update manager "Available Updates" page, with is a big UI change for all users. According to the core changes policy:
I do not see a new group without any description in place as a same behavior. Users will be confused what does the group "Other" mean and what to do with it. It will contain both enabled and disabled modules which is wrong from the usability point of view.
Maybe something like this could be more feasible approach in this D7 stage, just to allow some rows in a tableselect to be disabled: #2895352: Allow tableselect element options to be disabled. Then it can be used by the
update_advancedmodule to disable rows of modules which are set as ignored by this module. Assuming, of course, that there is still an effort to resolve this issue, which is more or less limited to thisupdate_advancedmodule (please correct me if I am wrong).Moving this to Needs Review for some feedback on this.
Comment #43
poker10 commentedJust for the info, #2895352: Allow tableselect element options to be disabled was commited to D10/D9, so if it helps to solve this problem (as an alternative approach), then we can create an issue for D7 backport and try to solve it that way. I have briefly checked the issue and it seems that it should be possible to do a D7 backport.