Repository navigation
Act on the real article name after a confirm box - #37
Merged
Merged
Conversation
confirm_box() posts back to an address whose path is encoded twice, so an article called "Some Article" arrived as "Some%20Article" and matched nothing. Deleting an article and switching one off reported success but changed nothing, and deleting an uploaded file said the file did not exist, for every article name containing a space. The handlers now use the name the box carried in a hidden field. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Fixes three actions that silently failed for any article whose name contains a space (most of them):
The cause:
confirm_box()posts back to an address whose path is encoded a second time, so the route gaveSome%20Articleinstead ofSome Articleand every query on the name matched nothing. The handlers now take the name from the hidden field the confirm box already carries (functions::confirmed_article()), falling back to the route's name on the first request, before the box. Names without a space were never affected, which is why earlier tests, which used underscores, did not see it.The delete handler gains the request object as a constructor argument for this (its service definition is updated).
Testing
Claude reproduced all three failures on a phpBB 4.0 (master) board seeded by the new wiki fixtures: the article delete showed success with the row still in the table, and the file delete answered "The file does not exist." After the change, on the same board and on throwaway phpBB 3.3.19 boards with MySQL 8.0 and with PostgreSQL 16, a browser run confirmed that an uploader can delete their own file from "Markup Guide", a moderator can set "Bulk Article 06" inactive, and a moderator can delete "Markup Guide", whose file rows and files go with it (5 checks per board, all passing). Four new unit tests cover the helper: the posted name wins over the route's, the route's name is used before the box, HTML escaping is undone, and Unicode survives. All 66 wiki unit tests pass.
Investigated and written by Claude on behalf of William Jacoby (bonelifer).