qtranslate / qtranslate-xt

qTranslate-XT (eXTended) - reviving qTranslate-X multilingual plugin for WordPress. A new community-driven plugin soon. Built-in modules for WooCommerce, ACF, slugs and others.
GNU General Public License v2.0
546 stars 103 forks source link

Add PHP type declarations #1314

Closed herrvigg closed 1 year ago

herrvigg commented 1 year ago

PHP type declarations make the code clearer and catch some bugs due to wrong calls not handled properly. PHP7.1 is interesting for nullable args and void return values. See PHP type declarations.

This patch covers all of the qTranslate code. Though it doesn't enforce strict checks yet, this has little effect with array and strings. These changes might break some parts in specific situations so it should be tested by developers in master. Some types are left unspecified, as mixed types require PHP 8.0 and some WP/plugin APIs are unclear.

herrvigg commented 1 year ago

@spleen1981 I put you as reviewer, but you don't need to go through all of the code as it's quite massive. Maybe take a more precise look at modules / slugs, but it's mostly about testing. If some type is wrong it will trigger some error at runtime. After that we'll leave it for test in master for a few days.

herrvigg commented 1 year ago

So, I've tested the main functionalities and it holds well. Let's merge and see now in master, we'll fix it if needed. With the 3 sub-commits it might be easier to check back later if needed.

herrvigg commented 1 year ago

Strong type checks with PHP is the way to go because it helps to debug by finding wrong usage in the API calls. The drawback is that is it can create many regressions because it's impossible to test all the combination with integrations and hooks.

It's unfortunate the WP documentation is not accurate and their APIs are so messy with many mixed types often undocumented 😕 Let's try to stick to safe usage but it's difficult to judge where to set the boundary. The checks in core functions should be kept. But I may revert type checks that are risky, typically when plugged with hooks.

herrvigg commented 1 year ago

Great. I reverted/fixed quite a few things related to hooks with WP / other plugins, but slugs is mostly internal so we should be able to control the types better.