Skip to content

Fixed E_WARNING "($context) must be passed by reference, value given". #39

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

Conversation

mumumu
Copy link
Member

@mumumu mumumu commented Jan 27, 2021

When I use PhD with PHP 8.0.x, I get the following warning flood.

[17:55:07 - E_WARNING             ] /home/mumumu/.phpenv/versions/8.0.1/lib/php/phpdotnet/phd/Format.php:150
        phpdotnet\phd\Format::SQLiteExample(): Argument #1 ($context) must be passed by reference, value given

I can't find why this warning occurs, but $this pseudo value is NOT a reference!! ( php/doc-en#380 )

This PR works on PHP 7.4.x, 8.0.x.

@IMSoP
Copy link

IMSoP commented Feb 28, 2021

The warning is nothing to do with $this, but I believe this fix is correct.

The warning comes when the SQLite3 createAggregate method executes each of the callbacks passed here: https://github.com/php/phd/blob/master/phpdotnet/phd/Format.php#L138 The extension passes a value not a reference as the first parameter to those callbacks, triggering the warning.

Since none of the callbacks actually makes use of that first parameter, except for SQLiteFinal which returns it unchanged, it should be perfectly fine to change signatures to accept it by value, as this PR does.

@kamil-tekiela kamil-tekiela mentioned this pull request Jun 18, 2021
@cmb69
Copy link
Member

cmb69 commented Jun 18, 2021

Since none of the callbacks actually makes use of that first parameter, except for SQLiteFinal which returns it unchanged, it should be perfectly fine to change signatures to accept it by value, as this PR does.

Right, but that class might be extended, and we can't control that.

Also, we need to cater to

"php": ">=5.3.0",
(should probably have raised that long ago).

@Girgias
Copy link
Member

Girgias commented Jun 18, 2021

I think the first step is to raise the project dependency to 7.2 (which gives us most useful typing structures).

@kamil-tekiela
Copy link
Member

Do we actually know if anyone has created any extensions to this project? Did anyone ever try to override these methods? What would happen if they did so? Does the pass by reference do anything ever?

@nikic
Copy link
Member

nikic commented Jun 18, 2021

I'd suggest putting a note in the README that this project has no API stability guarantees whatsoever and go wild :)

I don't think we (the PHP project) benefit from anything else.

@cmb69
Copy link
Member

cmb69 commented Jun 18, 2021

Can we remove the package from PEAR and packagist?

Copy link
Contributor

@salathe salathe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaring these methods as taking the $context parameter by reference is a bug, as evidenced by the E_WARNINGs. If (and it's a big if !) anyone extends these methods in their own code (we don't in phd), their code has a bug and should be corrected.

TL;DR - IMO, don't sweat it, fix the signatures.

@salathe
Copy link
Contributor

salathe commented Jul 8, 2021

Can we remove the package ...

The following addresses the question of "can we remove", and not "should we remove" (though my answer to the latter would be "sure").

... from PEAR ...

The PEAR package is in the doc.php.net channel (rather than the usual pear.php.net channel), which is managed via the php/web-doc Git repo.

... and packagist?

I don't see a way to remove the php/phd package from Packagist, but have marked it as "abandoned". @Seldaek is there a self-service way to remove that package, or would someone from your side need to remove it?

@Seldaek
Copy link

Seldaek commented Jul 12, 2021

I removed the package from packagist.

@salathe
Copy link
Contributor

salathe commented Jul 12, 2021

Thanks for the assistance, Jordi. 👍

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.

8 participants