Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix table backup + delete button on settings/roles/edit/, developer/logs:delete_all etc. by restoring post_key_exists() #699

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

sourcejedi commented Dec 23, 2012

For the cause of the issue, see be42cff#commitcomment-2349113.

Don't get me wrong :). I wasn't offended by the removal, and I'm happy to see alternative suggestions. post_key_exists() seemed to be the least bad solution. I've revised the API comment to explain the reasoning: 37f1b4f.

(Again, I'm not as set on this as the tone of the API comment might suggest - obviously the comment is written with the assumption that the commit is acceptable and gets merged).

sourcejedi added some commits Dec 23, 2012

Revert "Removing all post_key_exists since it duplicates existing CI …
…functions and forces another file to be read for every page load."

This reverts commit be42cff.

Justification should be provided by API comments added in the following
commit.
post_key_exists(): add missing justification
The originally committed comment described the pattern of use,
in Bonfire, but failed to spell out why it required a new method.
activities module: slightly better comment
I couldn't remember how (or whether!) the delete button
scripting worked.  So I checked it, and found the comment
was not as explicit as it could have been.
Contributor

itsravenous commented Feb 28, 2013

FWIW, this would be useful for me - I use for all my submit buttons, so that I can put icons, etc in them easily. Not having to put a value attr on the button would be nice, as all Bonfire needs to know is that the post key is there (i.e. the button was used to submit the form), not whether it contains any content.

Contributor

sourcejedi commented Feb 28, 2013

(I.e. "I use <button> for all my submit buttons". Markdown ate the html markup). Disclosure: itsravenous isn't my sock-puppet, but I did point him here.

@sourcejedi sourcejedi referenced this pull request Feb 28, 2013

Closed

Installer improvements #705

Contributor

sourcejedi commented Mar 19, 2013

Edit: this also breaks the creation of database backups.

sourcejedi added a commit that referenced this pull request Mar 22, 2013

Revert to use of isset($_POST[]) for submit buttons
Fix the unfortunately incomplete revert of post_key_exists().
See #699 for the known issues, mainly nonfunctional delete `<button>`s.
Also reported as #755.

post_key_exists() was intended to replace, unify and name the necessary
usage of isset($_POST[]).  This attempt drops the replace and name part :).

I assumed post_key_exists() was already in the right places, and ran a
global search+replace on it.

git checkout -b tmp
git revert be42cff
git diff -w > revert.patch
git checkout develop
git branch -D tmp

patch -p1 < revert.patch #+1 fix of rejected hunk

git rm application/core/MY_Input.php

find -name *.php -exec sed -e "s/\\\\\$this->input->post_key_exists('\([^']*\)')/isset(\\\\\$_POST['\1'])/g" -i \{\} \;

find -name *.php -exec sed -e "s/\$this->input->post_key_exists('\([^']*\)')/isset(\$_POST['\1'])/g" -i \{\} \;

git grep post_key_exists

@sourcejedi sourcejedi closed this Mar 22, 2013

Contributor

sourcejedi commented Mar 22, 2013

(Perhaps I was being too picky about not using $_POST directly for this. Hopefully that can be accepted by the other devs).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment