-
Notifications
You must be signed in to change notification settings - Fork 738
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
Improve filter documentation #3941
Conversation
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.
Thank you very much! A welcome clean-up.
This looks basically already good to me (maybe move the flags on the constants page below the validation and sanitation filters). I'm even fine with keeping the rest of the filters page for now; already quite something to figure out for translators.
Co-authored-by: Christoph M. Becker <[email protected]>
@@ -3,261 +3,6 @@ | |||
<chapter xml:id="filter.filters" xmlns="http://docbook.org/ns/docbook" xmlns:xlink="http://www.w3.org/1999/xlink"> | |||
<title>Types of filters</title> | |||
|
|||
<!--Validate filters: {{{--> | |||
<section xml:id="filter.filters.validate"> |
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.
Why was this section removed? Someone asked about this on Stack Overflow now https://stackoverflow.com/questions/79219053/why-are-the-php-validate-filters-not-documented-on-php-net
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.
The commit message for 6ab1e5f notes that the validate filters have been moved to the constants section, but I cannot find any reasoning behind doing this only for validate filters. There is also no indication in the docs that the validation filters are documented elsewhere.
IMO this commit should be reverted and a new PR filed with specific justification for review.
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 disagree, the other sanitization filters should be integrated into the constant page.
The current documentation was out of date, out of sync, and somewhat ill-defined.
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.
The documentation may have been out of sync, but I feel like https://www.php.net/manual/en/filter.filters.php was used more than the constants page. It was much easier to read than a long list of organized constants. The constants page should be nothing more than a list of defined constants and the actual functionality should be documented here.
What we have ended up now is just confusing. Part of it is documented on constants page and part of it remains here. If it were done all at once it would have been less of a problem. Right now it feels like we've lost a page in the manual.
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 have structured the constants to be grouped together, so that it is not "just a list" of random constants.
Previously, the constant page was utterly useless, that's why no one used it.
I am happy to work on it again, but would help if other people would help document (especially PHP 8.4) instead of nearly being the only one doing stuff to improve the state of the manual.
So I am going to veto any revert of this PR. As incremental improvements are fine instead of requiring everything to be done in one huge PR which also affects translators.
Organize constants somewhat and add descriptions.
This is still somewhat W.I.P. but the plan is to effectively integrate the information of
reference/filter/filters.xml
.I'm not sure how to document the options yet, as I'm somewhat unfamiliar with the extension.