Skip to content

Fix PHP warnings #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

kamil-tekiela
Copy link
Member

This fixes the annoying:

[21:40:50 - E_WARNING ] D:\projects\phd\phpdotnet\phd\Format.php:150
phpdotnet\phd\Format::SQLiteFinal(): Argument #1 ($context) must be passed by reference, value given

I don't know if pass-by-ref was needed in previous PHP versions, but the current documentation says it's a normal parameter.

@cmb69
Copy link
Member

cmb69 commented Jun 11, 2021

[…], but the current documentation says it's a normal parameter.

Where are these docs?

Regardless of docs this change is a BC break, and needs to be handled as such; not sure what exactly that means, tough.

@kamil-tekiela
Copy link
Member Author

I meant this docs https://www.php.net/manual/en/sqlite3.createaggregate.php

Why would that be a breaking change? Because the function is a public function? I think we can make them private, SQLite3 shouldn't complain about that.

@cmb69
Copy link
Member

cmb69 commented Jun 11, 2021

The phpdotnet\phd\Format::SQLite*() methods are supposed to be overrideable, I think. I also think that the signature in the docs is wrong, since they also state: "Your PHP function should accumulate the result and store it in the aggregation context."

@kamil-tekiela
Copy link
Member Author

kamil-tekiela commented Jun 11, 2021

and store it in the aggregation context.

Which is then returned from the function. Anyway, if the manual is wrong why does PHP throw a warning due to invalid signature.

I don't know how this project works. If there is a chance that someone overrides these methods then we should handle it more carefully, but I see no way to handle it differently right now. A bug is a bug and it needs to be fixed. If it breaks someone else's code then they need to fix that bug too.

@kamil-tekiela
Copy link
Member Author

Duplicate of #39

@kamil-tekiela kamil-tekiela marked this as a duplicate of #39 Jun 18, 2021
@kamil-tekiela kamil-tekiela deleted the fix-warnings branch June 18, 2021 14:48
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.

2 participants