textpattern / textpattern

A flexible, elegant, fast and easy-to-use content management system written in PHP.
https://textpattern.com
GNU General Public License v2.0
768 stars 111 forks source link

`Implicitly marking parameter $atts as nullable is deprecated, the explicit nullable type must be used instead` with PHP 8.4.0alpha1 #1931

Open petecooper opened 1 week ago

petecooper commented 1 week ago

https://php-next-demo.textpattern.co

dev with PHP 8.4.0alpha1 throws the following on most admin-side pages (not Diagnostics) and front-side:

8192 "Textpattern\Tag\Registry::process(): Implicitly marking parameter $atts as nullable is deprecated, the explicit nullable type must be used instead"
in /var/www/servers/textpattern.co/php-next-demo/live/dev/textpattern/vendors/Textpattern/Tag/Registry.php at line 126.

Admin-side follow-up:

textpattern/vendors/Textpattern/Loader.php:154 adminErrorHandler()
textpattern/vendors/Textpattern/Loader.php:154 require_once()
textpattern/vendors/Textpattern/Container/Container.php:120 Textpattern\Loader->load()
textpattern/vendors/Txp.php:54 Textpattern\Container\Container->getInstance()
textpattern/lib/txplib_misc.php:1812 Txp::get()
textpattern/vendors/Textpattern/Search/Method.php:137 lAtts()
textpattern/vendors/Textpattern/Search/Method.php:121 Textpattern\Search\Method->setOptions()
textpattern/vendors/Textpattern/Search/Filter.php:122 Textpattern\Search\Method->__construct()
textpattern/vendors/Textpattern/Search/Filter.php:102 Textpattern\Search\Filter->setMethods()
Textpattern\Search\Filter->__construct()
Screenshot 2024-07-02 at 17 42 57

Front-side follow-up:

textpattern/vendors/Textpattern/Loader.php:154 require_once()
textpattern/vendors/Textpattern/Container/Container.php:120 Textpattern\Loader->load()
textpattern/vendors/Txp.php:54 Textpattern\Container\Container->getInstance()
textpattern/publish/taghandlers.php:30 Txp::get()
textpattern/publish.php:224 include_once()
index.php:76 include()
Screenshot 2024-07-02 at 17 43 06

I'll check the PHP logs to see if there's anything useful.

Bloke commented 1 week ago

So this kind of forces our hand to either: a) drop support for PHP 5 (or, in fact, anything below PHP 7.1) because we can't backfill code with ?type for anything below PHP 7.1; or b) do what Oleg did and comb the code to remove all uses of null as a default value for function parameters.

Both are a ballache, which is why many places are suggesting using cs-fixer or Rector to automatically update code. Not sure I like the idea of some tool yomping through our codebase and changing stuff, especially for stuff we've brought in through the /vendors route. But doing it by hand is hard graft and boring so... dunno.

Either way, Txp 5 will need a significant minimum ride height jump, I expect.

bloatware commented 1 week ago

There seem to be only 4 remaining occurrences, we can fix them manually. And switch to php 7.1 in txp 5, indeed.

Bloke commented 1 week ago

Oh cool. I figured it'd be used everywhere. If there are only 4, that's a cinch. Phew.

(I expect it's probably used in conditional plugins a lot, to set $thing = null so all these may need tweaking).

bloatware commented 1 week ago

As long as $thing is not typed, that's not a problem, I think.

Bloke commented 1 week ago

Even better. Variable types: pah! Just reassign variables as any type, willy nilly, and live with the consequences of sloppy coding ;)

So why did fb48bbbfc stop it complaining then? The parameters and function aren't typed... are they?

bloatware commented 1 week ago

I guess php 8.4 does not like an array to have null default value, like in array $atts = null? But array $atts = array() is okay. Seemingly, in our code we do not check specifically whether $atts is provided, so the fix looks easy.

Bloke commented 1 week ago

Ah okay, hence 'implicit' in the error message. Maybe they're internally typing anything passed as an array as... an array! Which triggers the warning if we're defaulting it to some other type. Makes sense, I guess.

bloatware commented 1 week ago

No idea what to do with nullable class type declarations, like Skin $skin = null, to please both php 5 and 8.4. The former won't accept ?Skin $skin nor Skin|null $skin = null, neither Skin $skin = new Skin(). Should we simply remove type declarations here, awaiting php 7.1 switch?

Bloke commented 1 week ago

Ugh, yeah, maybe remove the type declarations from the Skin class for now and we'll reintroduce it in Txp 5 when we have a higher PHP baseline.