search

Automattic / Edit-Flow

WordPress plugin to accelerate your editorial workflow
https://editflow.org
359 stars 132 forks source link

Lint the various modules #734

Closed ingeniumed closed 4 months ago

ingeniumed commented 5 months ago

Description

This PR is meant to lint all the PHP classes at once to make development easier. I have skipped the commons folder, just like we skipped it for the JS linting in the interest of not breaking everything. We should revisit that decision or try to refactor it, but I'll set that aside for now.

There's also a bug fix here as the settings updated css wasn't being applied correctly. That's resolved now.

ingeniumed commented 4 months ago

@ingeniumed Thank you so much for doing these changes! There were a ton of really valuable updates to escaping functions, translator strings, and isset checks, as well as a lot of code-quality updates like adding braced and correcting code spacing. I found turning whitespace off (settings gear -> hide whitespace) in the diff was very necessary for review. Note I also tested features manually in this branch to ensure nothing obvious was broken, and everything loaded and ran well.

I left comments on specific issues I noticed above.

There was one big change we may want to reconsider (mentioned once here), and that's the switch from date() to gmdate(). This was done in a lot of places. gmdate() is the correct function to use generally speaking, but these functions return different values.

Unless we're really careful to migrate date calculations correctly, this could cause scheduled posts and other date-related features (like the calendar) to suddenly change and be off by a few hours. I think users typically expect WordPress to reflect the date and timezone set in Admin -> Settings -> General, and modifying our core date function without careful testing could cause unexpected behavior in multiple features.

I don't know of any specific place of where this will break, but I think I could find some if you'd like a more concrete example.

What do you think?

Thanks for reviewing it, this isn't an easy PR to review so I appreciate it.

I think out of safety, I'll revert that change and exclude that rule from phpcs ignore for now. In a follow up PR, we can do rigorous testing and get that in. While I did test it briefly, and it worked well I don't think we should risk letting it go in in this giant PR. You are 100% right on it imo