pfSense » pfSense Packages

I reverted the broken commit. Leaving this as resolved for now (though "Needs patch" may be better).

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.)

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.

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.