Add PHP type declarations #1314
Conversation
|
@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. |
herrvigg
force-pushed
the
php-types
branch
2 times, most recently
from
a7bfe28 to
3f65023
Compare
|
So, I've tested the main functionalities and it holds well. Let's merge and see now in |
PHP type declarations make the code clearer and catch some bugs due to wrong calls or usage. PHP7.1 is interesting for nullable args and void return values. This patch doesn't enforce strict checks yet, but 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.
Fix wrong type in `qtranxf_adjust_config_files`. Complete missing trivial types. Improve robustness of `rest_request_after_callbacks` with type check.
Remove arg type declaration as this function must also accept `object`.
The main purpose is to filter `redirect_canonical` (WP hook). The WP documentation is not precise regarding the types so we have to relax the types check. Check first if the redirect URL is false. The second parameter is the requested URL but it is ignored. See #1320, wrong usage with Yoast. The second parameter has a different meaning with Yoast. Removing the types will work but the Yoast hook should be rewritten with a different function. Rewrite the function to handle the default language in convertURL.
See also #1320. It is too risky to specify these types for now.
|
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. |
Enabling too strict type checks can create regressions with hooks that are not well documented. Revert the type checks for hook callbacks that could be risky.
|
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. |
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.