Skip to content

Database exporter fix for php 8.1 deprecations#264

Closed
alikon wants to merge 1 commit intojoomla-framework:2.0-devfrom
alikon:patch-3
Closed

Database exporter fix for php 8.1 deprecations#264
alikon wants to merge 1 commit intojoomla-framework:2.0-devfrom
alikon:patch-3

Conversation

@alikon
Copy link
Contributor

@alikon alikon commented Jun 1, 2022

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
Copy link
Contributor

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
Copy link
Contributor

nibra commented Jul 3, 2022

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

@wilsonge
Copy link
Contributor

wilsonge commented Jul 4, 2022

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

@nibra
Copy link
Contributor

nibra commented Jul 4, 2022

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
Copy link

@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
Copy link
Contributor

nibra commented Feb 22, 2023

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

@mbabker
Copy link
Contributor

mbabker commented Feb 22, 2023

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
Copy link
Contributor

nibra commented Feb 22, 2023

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
Copy link
Contributor

Hackwar commented Aug 29, 2023

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

@alikon
Copy link
Contributor Author

alikon commented Aug 29, 2023

closing in favour of #264
better to have 1 pr

@alikon alikon closed this Aug 29, 2023
@alikon alikon deleted the patch-3 branch August 29, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants