Bug #4895
closed
System Patches - multiple regressions with the new code
0%
Description
1/ When you click Test, then click Apply without clicking Close on the red banner, it applies the patch, but does not reset the URL so it stays at http(s)://<FQDN>/system_patches.php?id=<patchid>&act=apply. Then, when you go to some edit other patch and click Cancel, it re-applies the previous patch! :-( Similarly, press Test, then Edit some other patch without clicking Close, it goes back to test previously tested patch again though this is obviously not as bad as the previous case.
2/ This does not handle creating of new files via patch.
Examples:
- http://github.com/pfsense/pfsense/commit/d9fcbc2048041285690cc28159753acdb0bbb7a3.patch
- http://github.com/pfsense/pfsense/commit/4dda40b25ee72d899add9fb18e41310dfcc53a17.patch
This shows perpetually "Valid, not applied" and does not allow revert at all, so if you accidentally apply it multiple times, you need to delete the file from console. The regression here being that previously, you at least were able to use revert action, this is no longer the case. When both actions are valid, it at least should show both Apply and Revert actions in that column.
Updated by Jim Pingle almost 10 years ago
I reverted the broken commit. Leaving this as resolved for now (though "Needs patch" may be better).
Updated by Jim Pingle almost 10 years ago
- Status changed from New to Resolved
Some notes on Kill Bill's observations and the assumptions made by the commit:
- Apply and Revert status cannot be combined because they are not always mutually exclusive. Certain commits can be applied multiple times or reverted multiple times depending on context. Some of those are unintentional but not all. Plus the file issues noted above are a problem.
- The combination of the columns already serves as a status, logic to make that more "human readable" would need to make assumptions that may not necessarily be true for all cases. It may be right more often than not, but when it's incorrect it could be very confusing to the user.
Though the attempt was appreciated, I'm not certain any of the changes are ultimately beneficial or necessary. We use this package frequently internally and it's best if it stays simple and working as it is.
Updated by Stilez y almost 10 years ago
I'm not sure about some of the comments above - I'll look into the points raised. This patch should only have changed visual layout - it shouldn't have made any change to GUI redirection/URLs, or the ability to create new files by patch (or remove them by reverting).
Jim - a couple of your points I don't understand fully and would appreciate an example as I can't think of meaningful examples at the moment:
Can you give me a simple example of when one might need to apply a patch and then re-apply that same patch to the already-patched code (accidental or otherwise)? I'm trying to think of any time that a patch could be doubly-applied (applied and then applied a second time to the already-patched code), or where the same patch with context can be both applied and reverted.
As a non-null patch by definition describes changes to the codebase, I would understand a patch to have a trinary state - can be applied forward (hence not backward as the lines required won't match), can be applied backward (hence not forward as again the lines won't match), or doesn't match the code and can be neither applied or undone. As I understand it, you're saying that's mostly but not always true, but I can't think of a situation it wouldn't be true. After all, that's what the patcher actually does, it can apply a patch, revert it, or indicate via return code it wasn't able to do these things. Can you clarify a bit what exceptional case might exist that needs to be taken note of?
(On reflection, is the issue that some patches are entered (or could be entered) with no context, so that they just list additions/removals, without reference to a means of checking the file is suited to these changes? If so, that itself would perhaps be a weakness.)
Updated by Jim Pingle almost 10 years ago
The "without context" parts are part of it, also if there are only additions and no deletions, with the right context/fuzz it can apply multiple times.
It's not about the examples you can think of though, it's about the ones you can't. If you limit the ability of the GUI in any way from the base utility, you run the risk of preventing someone from using a feature or expected behavior that is still technically valid even if you've never thought to use it that way.
As it is, it works well and those sorts of scenarios also function. There are times when sacrificing some edge case functionality for user-friendliness makes sense, but it's not here in this package. This is a "power" utility made for developers, not typical end users, so it's OK that it has a learning curve.
I appreciate the attempt to improve things, but given our dependence on this package and all its functionality, I'd rather keep it as-is for now.
Updated by Stilez y almost 10 years ago
Given this information, I think it's possible to meet both sets of uses neatly.
I'll have a go at a second version to accommodate the helpful new information raised, as I think it can be done in a way you might like, while checking carefully for issues raised, and without disrupting the internal use described or being confusing on the rare case you mention. If it still doesn't work I'll give up, but in light of GUI usefulness, I'd like not to merely give up first time :)
The first issue (staying at &apply= or &revert= rather than changing URL) is inherent in the current code. I've looked and there's a hiccup in fixing it - it really wants <$savemsg> to be propagated across the "header(location:)" change of URL from "system_patches.php?act=apply...." to "system_patches.php", but doing so will lose the GUI feedback held in {$savemsg}, as currently written, since it won't yet be in the form. While minor, I'd like to fix the point that GET parameters such as "act=apply" aren't got rid of when the request is serviced, and are re-processed incorrectly if one edits and cancels a new patch etc. Not hard to fix.