Update code to comply with WP Coding Standards (*fourth* time's the charm!) #50
Conversation
…gfix. = 2.5.6 = - Bugfix: Reference container: optional alternative expanding and collapsing without jQuery for use with hard links, thanks to @hopper87it @pkverma99 issue reports. - Bugfix: Alternative tooltips: shrink width to short content. - Update: Documentation: slightly revise or update the plugin’s welcome page on WordPress.org. git-svn-id: https://plugins.svn.wordpress.org/footnotes/trunk@2478170 b8457f37-d9ea-0310-8a92-e5e31aec5664
This is a squashed copy of `wp-coding-standard--rebase` to try and fix the line-ending diff issue we're having.
Not doing so will result in a `WordPress.WP.EnqueuedResourceParameters.NotInFooter` warning from PHPCS.
Merge deleted docblock attributions in remaining docblock.
Update: Libraries: Load jQuery UI from WordPress, thanks to @check2020de issue report. Under 2.0.4.
Name variables in an intuitive manner according to coding conventions.
Fix header docblock as discussed in #38 (comment)
Fix 2 function docblock headings by dispatching them from the originally single metabox function docblock. Also add version tags to the 2 metaboxes split off @SInCE 2.2.0. * Displays all options for the footnotes start and end tag short codes. (Initially called “Footnotes styling”.) * Displays all options for the footnotes numbering. (Newly individualized.) * Displays all options for the scrolling behavior. (Totally new as scroll offset and duration were initially hard-coded in the template.) (This is part of WP coding standard compliance.)
Titlecase “Translators:”. Add semicolons.
Headings and version for split-off metaboxes docblocks. Overwrites the todo items. Thanks for starting the docblocks required by WP coding standards.
The historic repo where @felipelavinz contributed has recently been moved. As a consequence the commit’s URL needs to be updated. But the original URL is maintained below under the commit date.
My mistake, I misunderstood where you were having the issue.
Agreed, the use of output escaping is not necessary within the context of this Plugin. For now, I've removed all of the output escape and input sanitisation functions in 25c3f2f (you can check by running
The use of file-level suppression comments solves this.
Fixed in 25c3f2f
Incorrect. The scope of this PR is to add pre-commit linting to enforce compliance with the WP Coding Standards. The linter raises errors for unescaped output, and so solving that issue (whether by escaping output or suppressing the error) is within this scope.
As @markcheret has said elsewhere, the current priority is getting the codebase up-to-snuff (provided there are no critical/security fixes to implement).
As stated above, the scope of this branch is clear: implement pre-commit PHPCS validation to enforce compliance with the WP Coding Standards, and ensure that all code passes. This necessarily involves a huge number of minor changes (e.g., double quotes to single quotes), but it also requires a few manual changes that are more impactful and have the potential to introduce problems (e.g., the output escaping). These manual changes are where issues may arise, but they make up a minority of the overall changeset.
Again as per @markcheret—excluding any critical/security issues that may arise—merging this PR is currently the priority. Once we've done so, we can return to regular bugfixing and adding features on the standardised codebase (rather than maintaining two variants—it required a non-trivial amount of effort to backport the 2.5.7 bugfix in #51 and that only altered 4 or 5 lines). As the standardisation of the codebase will not merit a new version of the Plugin, we will still have plenty of time still to catch any other issues that may slip through during subsequent normal development, though between your thorough code review and the reversion of output escaping in 25c3f2f (which was one of the most widespread impactful manual changes made) I think that is unlikely. In a worst-case scenario, even if something really obscure breaks and we don't notice until we get a bug report in five months time, then that's something we can fix then and there easily enough. Only 50% of users are on the latest version, after all. |
No problem.
Thank you for fixing the escapement problem.
File level configuration seems a good compromise between project-wide and local.
Thanks!
WordPress Coding Standards does not seem to recommend Lint. The compiler catches bugs, while a linter seems to enforce general or common-sense rules that may, as you state, not apply to a given codebase.
I tried to carefully evaluate @markcheret’s directives and didn’t suspect that I got him wrong, as he both mentioned “compliance” AND “Let's also continue what has been going on bugfixes and enhancements can be done alongside if the codebase permits.” And: “So, if there are improvements to publish, by all means, go for it and publish a tested version of new bugfixes or features.” The “alongside” is what allowed to release bugfix 2.5.7 in real time, which is also what I anticipated the possible need of.
I’m fine with that, no problem for me. It’s even easier to type.
I’m sorry that the linter or whatever tool got you to go to that trouble.
I’m glad if that’s all.
Thank you very much for integrating the 2.5.7 changes!
Indeed 2.5.7 was basically 1 line of code and 1 new docblock above it.
Seems unlikely, I think so too. Will just review the code again, hoping to catch any remainder.
Sigh. |
There was a problem hiding this comment.
I’m unable to review the codebase again, and it’s superfluous since @Rumperuu assessed all problems as solved.
Moreover the plugin escapes quotation marks in user input! It’s unusable for custom CSS involving HTML argument values. The plugin is still so buggy it would be shameless to submit it to AMP-WP as-is even after adding AMP compat.
RTL compat bugfix v2.5.8 needs to be fast-tracked instead, see the latest support issue.
|
@pewgeuges @Rumperuu is the escaping of quotation marks in the user input a new behaviour introduced by the refactoring here or has it historically been implemented? |
That's fine, 2.5.8 shouldn't be any bother to backport.
I'm not able to reproduce this on either Update: HTML elements within CSS are escaped; see here for custom CSS and here for output. This is also the case on |
Will be released in response to Forum and as agreed. = 2.5.8 = - Bugfix: Layout: support right-to-left writing direction by replacing remaining CSS values 'left' with 'start', thanks to @arahmanshaalan bug report. - Bugfix: Layout: support right-to-left writing direction by enabling mirrored paddings on HTML dir=rtl pages, thanks to @arahmanshaalan bug report.
I did backport it, even prior to releasing it, but the line end issue has come back. Why is this branch using other line ends, and what can I do to address the problem? Given this branch does not edit the style sheets, I pasted the modified ones over the existing ones. E.g. 12 minified style sheets were changed. As of the readme.txt, I synced it alongside so it was handled the same way. Only footnotes.php was edited in the Git branch. Now all these files are jumbled again. Thank you @Rumperuu for renormalising the line ends f129f3a Note that SVN does not have this issue.
It is a longlasting bug that I noticed only now. My bugreport is based on the plugin as it is deployed.
Congratulations: You already fixed it, then! But how on
Yes I do always pull, but for the releases I needed to revert to the current plugin, and develop and test therein, until this branch has been merged. Then you will commit it to SVN updating trunk/, and then we’ll be able to use and release it with the next due release, as you planned, should any urgency arise before setting up the workflow in GitHub is completed.
Thank you for all the tests. That is not what I’m referring to, but |
Not to worry, however it happened f129f3a has fixed the ‘Files Changed’ tab view so we're good to go!
How strange, I'm getting the bug on Anyway, if there are no more issues identified with this branch by 20:00 UK time I will merge it into |
If Footnotes’ GitHub main branch is now having other line ends than its SVN repo, should this be updated WRT line ends so that the issue won’t be replicated there?
Will you commit the updated main branch again, for the changes to be visible on SVN? (WordPress don’t require to only commit release versions.) |
I've not touched the SVN repo. since I cloned it to here, so the SVN codebase will have never received 8d0ebdb.
So once this PR is merged, we'll effectively have two codebases: footnotes here in You have more experience with the SVN than I do, but am I correct in thinking that once the merge here is complete, we'll be able to just copy the files in In which case (for my own benefit when I come to #30), the steps for an automated release workflow would be:
Does that sound about right? |
If we can be sure that SVN is EOL agnostic, it may not need it. But given GitHub is not, I doubt SVN is. You are welcome to commit the EOL normalization and the standardised codebase. Hopefully SVN will then display a proper changeset, which I doubt it would without prior EOL normalisation.
Thank you for taking this step after you assessed all issues as solved!
I see a problem in doing so without previously normalising the EOL in SVN. Yesterday I tried to get hold of a method to normalize EOL but failed. You do have this method. Would you like to do the EOL fix? (And update trunk/ with main/?)
I think that will be great!
I would say yes, please go ahead.
For use in my local sandbox I’m currently deleting these files and folders: #!/bin/bash
# 2021-02-28T2217+0100
# gwp.sh
rm -rf GW/.git
rm -rf GW/.github
rm -rf GW/docs
rm -rf GW/_tools
rm -rf GW/.gitignoreIs that correct? The docs are twice as heavy as the whole codebase (1.6MB adding to 0.8MB), and as already reported they’re of limited use.
I think that this step may create more problems than it solves. The only file is settings.css, while all other stylesheets need to be concatenated first, as specified in development/csscat.sh, then these 12 files are minified, and then they’re deleted as pointless because the source files are shipped, along with the concatenation spec.
Beside the stable tag in readme.txt, there are two instances of the version number to be maintained in footnotes.php.
I don’t have enough experience to assess this process. I used to add or remove files one by one to/from version control in the SVN client, and commit. In the commit message, even double quotes enclosed in single quotes seem to need to be escaped, or SVN would fail to process a
That is correct, too.
With the abovementioned caveats, yes, sounds good to me. Your automation effort is surely much appreciated, although I wouldn’t ask for so much. Then there is also the pre-commit hook that you added support for. |
Learning Points from this PRThis took much longer than intended and broke my own suggestions in a) implement the automated code validation This would have ensured that we got ~80% of the way to a standardised codebase in a single sweep, whilst allowing the more difficult manual changes to be dealt with one-by-one in separate, less-monolithic and easier-to-review PRs. Something will certainly bear in mind in future workflow implementation (+ other projects).
Done! |
Description of Changes
Updates the codebase to ensure compliance with the WP Coding Standard.
Also Composerises the project to allow for the installation of a pre-commit PHP_CodeSniffer hook (as well as other hooks later on).
History
This is a copy of #38 after it was un-merged in #49.
Additional Notes
This branch's commit history will be squashed on merge, so do not make any commits to this branch that you're going to want to remain in the project history. Keep it scoped to WP Coding Standards compliance only.
Actions
Replace #38
Close #8
Close #10
Close #11
Close #46