-
Notifications
You must be signed in to change notification settings - Fork 63
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
GH-114: Lock down bug tracker to developers only #120
Conversation
All further conversation about bugs is supposed to happen on Github. We still allow developers to edit the bug tracker, so they can clean up. We start by disallowing users to add patches.
Note that I took (almost) no effort to clean up dead code (the whole CAPTCHA might not be needed anymore, and likely there is more dead code); that might be done later, or never be done at all. Otherwise I think this PR is ready for review. |
Although I'm not necessarily against doing this, I don't think it is good practise to argue for doing this on just a PR.
|
Fair point! I've sent a respective mail to php-webmasters and internals. |
Heh, why does that show as |
Maybe your email address is associated with that user? |
Does anybody have a developement environment for web-bugs set up? I'd rather not commit completely untested changes, but I still think that we should proceed here. |
I do. But I'm not back until later in the week.
|
No need to hurry. Thank you! |
If we only had tests 🤣
|
There are actually tests! |
I can't find any issues with this patch, but I am not sure if I have tested every path — could you perhaps list the ones, as there seem to be more than the four checkboxes. |
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.
LGTM, except for the single comment.
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.
Some comments about what the changes are supposed to do.
echo "<p><a href='patch-add.php?bug_id={$bug_id}'>Add a Patch</a></p>"; | ||
if ($logged_in) { | ||
echo "<p><a href='patch-add.php?bug_id={$bug_id}'>Add a Patch</a></p>"; | ||
} |
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.
Only show "add a patch" button for developers, now.
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.
Also tested, and fine.
There is also still a link "add pull request" in the same fashion though (line 1116 below), but I guess we can leave that one?
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.
This is an open issue in the OP's todo list. I think we should still allow to link PRs (they do no real harm), although that is debatable.
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.
I think this looks good. Merge it?
echo "<p><a href='patch-add.php?bug_id={$bug_id}'>Add a Patch</a></p>"; | ||
if ($logged_in) { | ||
echo "<p><a href='patch-add.php?bug_id={$bug_id}'>Add a Patch</a></p>"; | ||
} |
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.
Also tested, and fine.
There is also still a link "add pull request" in the same fashion though (line 1116 below), but I guess we can leave that one?
IMO, this is good as is. The issue about linking PRs could be addressed later, if need be. |
All further conversation about bugs is supposed to happen on Github. We still allow developers to edit the bug tracker, so they can clean up.
We start by disallowing users to add patches.
TODO:
Note that I do not have a development environment for web-bugs, so this is completely untested. Maybe somebody with a development environment can check this? (and perhaps even help to solve further tasks)