search

joomla-framework / database

Joomla Framework Database Package
GNU General Public License v2.0
28 stars 34 forks source link

Database exporter fix for php 8.1 deprecations #264

Closed alikon closed 1 year ago

alikon commented 2 years ago

Summary of Changes

used null coalescing operator

Testing Instructions

with Error Reporting setted to Maximum run php cli/joomla.php database:export --folder=tmp/

you got a lot of PHP Deprecated: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated in /shared/httpd/test/zzzz/libraries/vendor/joomla/database/src/DatabaseExporter.php on line 305

richard67 commented 2 years ago

Question is what will happen if we apply the fix and run an export and the import the exported file again? Will the import work and apply null values where necessary where we now will have empty strings with the fix?

nibra commented 2 years ago

Whether you print null or an empty string does not make a difference. In both cases, the result will be <field ...></field>.

wilsonge commented 2 years ago

We may need to differentiate though as those are different things in e.g. postgres

nibra commented 2 years ago

We may need to differentiate though as those are different things in e.g. postgres

Not in this case - it is about htmlspecialchars() not liking null as a parameter.

Llewellynvdm commented 1 year ago

@mbabker or @nibra what is the way forward here, I see more null issues and its probably only going to get worse. I already from @mbabker response on the registry issue can see we need to go over all of the framework to solve it. Almost seems like a RECTOR task, to at least to get a dry run report of the scale of the task.

Anyway any more feedback here will help, thanks!

nibra commented 1 year ago

In this case, the patch is ok and doing it's job.

mbabker commented 1 year ago

No, it's not. As George already pointed out, there is a huge difference between a database column storing a null value versus an empty string, and this patch makes it impossible to distinguish the two in an exported dataset.

nibra commented 1 year ago

Although you're right that NULL should be handled differently, does this patch not change the behaviour. The exporter has always delivered an empty string for NULL values.

Hackwar commented 1 year ago

I've created an alternative PR which properly handles NULL values: #286

alikon commented 1 year ago

closing in favour of #264 better to have 1 pr