Removal of Eval and amended unit test

[levelfivehub]

Hi @Maks3w

This is now branched off develop with your changes. With your approval we will need to close the other PR.

Thanks

[Maks3w]

Add a description about when the exception is thrown.

[levelfivehub]

Done

[levelfivehub]

Following our conversation can you re-review the unit test. For PHP Objects/Classes we are using the Zend\Json\Encoder and Zend\Json\Decoder.

[Maks3w]

This test should work without changes. Otherwise this PR introduces a BC Break.

You'll find why JSON Encoder is not the solution. The adapter emulate php native serialize/unserialize without call the native implementation.

JSON Encoded adapter is this https://github.com/zendframework/zend-serializer/blob/master/src/Adapter/Json.php

[levelfivehub]

If we keep this test and make the changes suggested, from what I see the 'unserialize' will still fail. Mainly due to __set_status: "PHP Fatal error: Call to undefined method stdClass::__set_state()". The testUnserializeObject is skipped because of this.

This was the main reason why I decided to re-work this adapter altogether. What are your thoughts?

[Maks3w]

I think the original issue is not resolved. This adapter has as objective serialize and unserialize using the PHP string format and not the JSON format.

[levelfivehub]

If we change the format, then this test wouldn't work unless we change the expected value.

[levelfivehub]

Making changes to reflect upon comments made.

[Maks3w]

Import the final class instead the only the namespace. This is the only class imported.

[Maks3w]

I don't think you'll able to complete the issue with the JSON Encoder proposal. However your refactor work of PhpCodeTest is good.

Could you put the changes on PhpCodeTest in a different PR?

[levelfivehub]

Hi Maks, This is done here: #6

I think we should at some point discuss/review this adapter in general.

[Maks3w]

Thx. What is the difference between PhpCode and Json adapters?

PhpCode produces code can be executed directly (write the result in a file and then include_once that file)

Json Produces code could be shared with different platforms and languages. It requires always an additional step of decode/encode the format

[levelfivehub]

With PhpCode is there a need to unserialize if that is the case?

[Maks3w]

I'm not familiar enough with php code generation.

I can imagine scenarios for transform an string to php objects without write to disk. But this is as dangerous as eval, so its not recomendable.

[levelfivehub]

eval should be avoided, I agree with this 100%.

[Maks3w]

TBH this adapter does things Zend\Code already do.

[marc-mabe]

@levelfivehub @Maks3w

The adapter exists for two reasons:

1. Serializing PHP data into a string than can be written to a file and be loaded into opcache
   -> In this case the method unserialize is useless but all adapters should have the same API
   -> It's different as ZendCode as it simply calls the build-in var_export which is really really fast
2. For debugging purposes to get a better human readable output. In mast of the cases JSON + pretty print would be a better adapter but in some cases you would like to have raw PHP class names visible which JSON can't ship.

Anyway this adapter has two big warnings in the documentation that it uses eval and __set_state (http://framework.zend.com/manual/current/en/modules/zend.serializer.adapters.html#the-phpcode-adapter).

[marc-mabe]

@levelfivehub I totally agree that eval should be avoided but this adapter is mainly for some special debugging only.

@Maks3w @levelfivehub @weierophinney @Ocramius I also would be fine by removing this adapter. Even if I see a use-case (explained above) this use-case is very very special and this adapter should be simple enough to write down if you really need it (I never did). Additionally shipping this adapter by default could result in using it for wrong purposes and opens which opens big security issues.

So mark it deprecated and remove in 3.0 - Thoughts?

Marc

[Ocramius]

This sort of adapter works really well with small cache maps, for example metadata related to entities, annotation cache, etc. Doctrine has a PhpFileCache that matches in functionality, and is really useful/performing in these scenarios. Not sure if that's relevant to the discussion, but I just wanted to bring it up, since file caches are quite OK for certain scenarios.

[marc-mabe]

@Ocramius you are right that in a scenario of file caches 'var_export' + 'include' works just fine but there the use case is a bit different because of the include statement. I mean it is fast because it will result in a two level cache - shared memory (by opcache) + filesystem.

In our case it can't use the opcache because it's using 'eval'. And because it can be used very simply outside of caching scenarios it's very simple to use this adapter for unserializing unsafe content.

We could add a PhpFile adapter to the caching component to ship this functionality.

[marc-mabe]

see #33