-
-
Notifications
You must be signed in to change notification settings - Fork 990
[ticket/10319] Missing hidden fields in search form #493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -4530,6 +4530,11 @@ function page_header($page_title = '', $display_online_list = true, $item_id = 0 | |||
{ | |||
$s_search_hidden_fields['sid'] = $_SID; | |||
} | |||
for ($i=0; $i<count($_EXTRA_URL); $i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use foreach here. Same applies to the other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foreach also works on "numbered arrays"
foreach ($_EXTRA_URL as $extra_url) { }
the for loop has several drawbacks:
- unnecesarry introduction of a variable $i
- calling count() on every iteration (we use sizeof instead of count by the way)
- code readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line before the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Concept-wise this $_EXTRA_URL stuff looks pretty fishy to me. |
Missing hidden fields in search form (bug added in 3.0.9) PHPBB3-10319
Looks good to me now, I'd merge this into the prep-release branch for 3.0.10 still. |
I agree that it looks fishy. Other way around it is to revert whatever change was added in 3.0.9 that changed all search forms from POST to GET, then this variable wouldn't be needed. |
He was only talking about the variable $i not about $_EXTRA_URL. This is perfectly fine. Edit: Oh I see Andreas' comment now. Anyway we must keep them GET so search result URLs can be copied, POST is not an option for these forms. I do wonder though if we have GET forms elsewhere that ignore $_EXTRA_URL. |
Nope, only 4 search forms were affected, 1 of them (in search.php) already has correct hidden fields. |
Cherry picked and merged into prep-release-3.0.10 |
Missing hidden fields in search form (bug added in 3.0.9)
PHPBB3-10319