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

Support pass-by-reference #225

Open
malkusch opened this Issue Nov 1, 2015 · 2 comments

Comments

Projects
None yet
2 participants
@malkusch

malkusch commented Nov 1, 2015

I really would love to integrate my php-mock with prophecy so that you could write prophecies about built-in PHP functions. This works already great, unless you want to use pass-by-reference (e.g. exec()).

You do obviously care about references in ClassCodeGenerator::generateArguments(). But they get lost on the further track. ProphecySubjectPatch::apply() uses func_get_args() which unfortunately doesn't preserve references:

$method->setCode(
    'return $this->getProphecy()->makeProphecyMethodCall(__FUNCTION__, func_get_args());'
);

If you don't mind I would prepare a PR which would not eat references in ProphecySubjectPatch::apply().

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 22, 2015

Member

the issue is that this would require using references everywhere in Prophecy (otherwise they would just be eaten one call later), which would then break things in all cases which should not use them (and might create lots of weird issues). And passing arguments by reference when they were not references initially causes a reference mismatch, which forces to duplicate the value, making things use twice the memory and being slower.

So IMO, this won't be supported.

Thus, I consider that reference-passing signatures are very confusing to use for many people. And they can often lead to performance issues due to reference mismatches (references are most of the time used when thinking that they would make things faster by avoiding to copy data, while a reference mismatch actually makes things worse, and this happens even to the Symfony core team).
So I would even consider that not being able to prophesize such API is a Prophecy feature rather than a bug (and @everzet might agree with me on this). This goes in line with the PhpSpec way: making it hard to work with badly designed APIs (according to the PhpSpec team definition)

Member

stof commented Dec 22, 2015

the issue is that this would require using references everywhere in Prophecy (otherwise they would just be eaten one call later), which would then break things in all cases which should not use them (and might create lots of weird issues). And passing arguments by reference when they were not references initially causes a reference mismatch, which forces to duplicate the value, making things use twice the memory and being slower.

So IMO, this won't be supported.

Thus, I consider that reference-passing signatures are very confusing to use for many people. And they can often lead to performance issues due to reference mismatches (references are most of the time used when thinking that they would make things faster by avoiding to copy data, while a reference mismatch actually makes things worse, and this happens even to the Symfony core team).
So I would even consider that not being able to prophesize such API is a Prophecy feature rather than a bug (and @everzet might agree with me on this). This goes in line with the PhpSpec way: making it hard to work with badly designed APIs (according to the PhpSpec team definition)

@malkusch

This comment has been minimized.

Show comment
Hide comment
@malkusch

malkusch Dec 22, 2015

this would require using references everywhere in Prophecy

It's quiet a time ago when I touched that code, but the fragments which I have lying around here aren't that invasive. It's very much focused around the offending func_get_args(). Anyhow, I would do the dirty work and you can just enjoy a further feature ;)

which would then break things in all cases which should not use them

I very much hope you trust your test suite to discover that. At least I do.

So IMO, this won't be supported.

Well, that's actually quiet appealing for me, cause it will safe myself some time and I would simply pass that statement further in my library's documentation. Could @everzet confirm that?

I consider that reference-passing signatures are very confusing

Maybe, and so is PHP's API. Unfortunately that is a hard fact which cannot be changed. If I want to enable prophecies for PHP built-ins, I have to cope with pass-by-reference no matter how confusing you may find them.

I would even consider that not being able to prophesize such API is a Prophecy feature

Then why does ClassCodeGenerator::generateArguments() care about them?

malkusch commented Dec 22, 2015

this would require using references everywhere in Prophecy

It's quiet a time ago when I touched that code, but the fragments which I have lying around here aren't that invasive. It's very much focused around the offending func_get_args(). Anyhow, I would do the dirty work and you can just enjoy a further feature ;)

which would then break things in all cases which should not use them

I very much hope you trust your test suite to discover that. At least I do.

So IMO, this won't be supported.

Well, that's actually quiet appealing for me, cause it will safe myself some time and I would simply pass that statement further in my library's documentation. Could @everzet confirm that?

I consider that reference-passing signatures are very confusing

Maybe, and so is PHP's API. Unfortunately that is a hard fact which cannot be changed. If I want to enable prophecies for PHP built-ins, I have to cope with pass-by-reference no matter how confusing you may find them.

I would even consider that not being able to prophesize such API is a Prophecy feature

Then why does ClassCodeGenerator::generateArguments() care about them?

malkusch added a commit to php-mock/php-mock-prophecy that referenced this issue Dec 24, 2015

Support Pass-By-Reference
To support prophecies for e.g. exec(), pass-by-reference is
required. Prophecy doesn't support pass-by-reference. This just
happens because ProphecySubjectPatch::apply() swallows them with
an offending func_get_args().

Upstream seems to be reluctant to change that behaviour as they
don't want to support pass-by-reference at all. Therefore I have
to overwrite the class ProphecySubjectPatch completely and patch
the desired behaviour in.

Overwriting classes is AFAIK a non intended behaviour of composer.
It may work accidentially but can change at any moment. I'll check
with composer if this could become an explicit feature. But for now
support for pass-by-reference is unreliable. If you need
pass-by-reference in prophecies, consider using another framework
(e.g. https://github.com/php-mock/php-mock-phpunit).

See also: phpspec/prophecy#225

malkusch added a commit to php-mock/php-mock-prophecy that referenced this issue May 13, 2016

Drop support for pass-by-reference
Supporting pass-by-reference was such a hack and now side effects of
that hack were reported. Upstream won't support pass-by-reference
anyways, so I'm going to drop that here as well. If you need to mock a
function which uses pass-by-reference (e.g. exec()) feel free to use
another testing framework (e.g. phpunit with php-mock-phpunit).

See also: #1, phpspec/prophecy#225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment