Feature: disallow serialization #1

Ocramius · 2016-10-11T04:06:33Z

This PR introduces:

Dont\Serialize
Dont\DeSerialize

TODOs:

code review
~~do Serialize and DeSerialize imply each other?~~ not for now, maybe in 2.x.
introduce humbug testing (mandatory)
documentation
repository description

…should be thrown

…n object

…r handling/catching

malukenho · 2016-10-11T04:18:17Z

src/Dont/DeSerialize.php

+     * @throws NonDeSerializableObject
+     * @throws TypeError
+     */
+    final public function __wakeup()


return type?

Can't: void is only 7.1, although we could make the lib 7.1 only.

👍 for 7.1

Another 👍 to allow : void return decl

Done with 7.1 bump

malukenho · 2016-10-11T04:21:33Z

src/Dont/Exception/TypeError.php

+     *
+     * @return self
+     */
+    public static function fromNonObject($object) : self


currently $object is not an object

Indeed, will fix!

malukenho · 2016-10-11T04:21:47Z

src/Dont/Serialize.php

+     * @throws NonSerializableObject
+     * @throws TypeError
+     */
+    final public function __sleep()


return type void?

Can't unless we force 7.1

Moved to 7.1

malukenho · 2016-10-11T04:25:52Z

src/Dont/DeSerialize.php

+use Dont\Exception\NonDeSerializableObject;
+use Dont\Exception\TypeError;
+
+trait DeSerialize


Can be unserialize instead

As discussed, deserialize would be proper English, while PHP has its own unserialize wording. Unsure about the wording to adopt for now. @asgrim, can you give us an opinion?

Better preserve php strangeness as it's more intuitive for php developers IMHO

We could always create an alias ? But i'm going to agree with @Ocramius for the simple reason I would be annoyed with phpstorm including the wrong thing.

Deserialise is the correct word. However you can have the version with the z if you must…

I could fancy a British lib...

Acceptable to me in selfish preference order: Deserialise Deserialize Unserialise Unserialize (note, no capital S in any)

Went with Deserialise, to be non-brexit-compliant

Deserialise is UK English btw, Deserialize is American, so technically, it's now Brexit-compliant 👍 🇬🇧

malukenho · 2016-10-11T04:31:13Z

src/Dont/Exception/NonDeSerializableObject.php

+The given object %s#%s is not designed to be de-serialized, yet de-serialization was attempted.
+
+This error is raised because the author of %s didn't design it to be de-serializable, nor can
+guarantee that it will function correctly after de-serialization, nor can guarantee that all


function mean work?

yes, it will function and it will work are synonyms

macnibblet · 2016-10-11T05:04:31Z

Whats the general purpose of this library ? Apart from the obvious :)

Ocramius · 2016-10-11T05:08:08Z

@macnibblet the idea is to:

final class MyFantasticService implements TheService
{
    use \Dont\Serialize;
    use \Dont\DeSerialize;
    use \Dont\Clone;

    public function __construct(Db $theDb) { /* ... */ }
    // ...
}

macnibblet · 2016-10-11T05:23:05Z

@Ocramius Copy from what I wrote in IRC, if you import the classes you end up with Serialize; Deserialize; Clone; I suggest you retain the Dont prefix in the trait names

asgrim · 2016-10-11T07:16:19Z

composer.json

@@ -0,0 +1,35 @@
+{
+    "name":        "roave/don-t",


grammar is hard, but IMO roave/dont is easier to remember package name

I can live with roave/dont

asgrim · 2016-10-11T07:16:41Z

composer.json

+    ],
+    "authors": [
+        {
+            "name":     "Marco Pivetta",


Typo in your surname there, should be Pivetti

asgrim · 2016-10-11T07:17:02Z

composer.json

+        }
+    ],
+    "require": {
+        "php": "7.*"


^7.0|^7.1 maybe, adding future versions as they appear?

In fact, mandating just ^7.1 could mean using void return type further down?

We can probably push to 7.1. This package is initially to be deployed as part of ocramius/proxy-manager for a version that only supports PHP 7.1+

asgrim · 2016-10-11T07:19:36Z

src/Dont/DeSerialize.php

+use Dont\Exception\NonDeSerializableObject;
+use Dont\Exception\TypeError;
+
+trait DeSerialize


Acceptable to me in selfish preference order: Deserialise Deserialize Unserialise Unserialize (note, no capital S in any)

asgrim · 2016-10-11T07:20:31Z

src/Dont/DeSerialize.php

+     * @throws NonDeSerializableObject
+     * @throws TypeError
+     */
+    final public function __wakeup()


Another 👍 to allow : void return decl

asgrim · 2016-10-11T07:21:06Z

src/Dont/Exception/NonDeSerializableObject.php

+
+use LogicException;
+
+class NonDeSerializableObject extends LogicException implements ExceptionInterface


Amend naming as discussed above (especially removing capital S)

asgrim · 2016-10-11T07:25:13Z

tests/DontTest/SerializeTest.php

+{
+    public function testWillThrowOnSerializationAttempt()
+    {
+        $object = new NotSerializable();


Do we need the assignment? just serialize(new NotSerializable()); on L22 should suffice

Didn't want to make it seem like the constructor caused the exception

asgrim · 2016-10-11T07:39:05Z

src/Dont/Exception/NonDeSerializableObject.php

+            throw TypeError::fromNonObject($object);
+        }
+
+        $template = <<<'ERROR'


I get the purpose, but these messages are really verbose... Surely a simple %s objects should not be (de)serialized would suffice? Make the point, but in less characters basically...

The idea of the lengthy message is to make this REALLY obnoxiously clear to the reader. There must be no issue opened by people asking why this kind of thing is being thrown (which is the reason why I started working on the package in first place)

asgrim · 2016-10-11T07:39:56Z

src/Dont/Exception/ExceptionInterface.php

+/**
+ * Marker interface for exceptions thrown by the Dont package
+ */
+interface ExceptionInterface extends Throwable


What advantage does this interface give?

It's a marker interface - provides generic "group by" and catch-all for the package, should anybody want to do that.

…pped when importing the trait, otherwise

asgrim · 2016-10-11T10:26:31Z

src/Dont/DontDeSerialize.php

+use Dont\Exception\NonDeSerializableObject;
+use Dont\Exception\TypeError;
+
+trait DontDeSerialize


Should be DontDeserialize imo (no cap on Serialize), the word is "deserialize" not "de-serialize" afaik

@asgrim

…e` as per @asgrim and @akrabat review

dantleech · 2016-10-11T10:43:35Z

tests/DontTestAsset/NotSerialisable.php

+
+use Dont\DontSerialise;
+
+final class NotSerialisable


Serialisable => Serializable (https://en.wiktionary.org/wiki/serializable)

Just moved to Serialisable as per @asgrim and @akrabat review.

both are correct: https://en.wiktionary.org/wiki/serialisable

I guess the s spelling is British, but PHP uses the z spelling.

Yup, we discussed already :) #1 (comment)

Yes, we just went with the brit approach:

Ocramius added 30 commits October 11, 2016 04:09

Basic phpunit setup

fac2859

Removing columns attribute (failed XSD validation)

f04434b

Basic composer.json describing roave/don-t purpose and dependencies

24dee13

Removed trailing comma (json, sigh!)

c18d168

Adding MIT licence

9d415b6

Autoloader should be PSR-4 based

821f34d

Spec for an exception marker interfe

157efd6

Implementing ExceptionInterface as per spec

e07615a

PHPUnit can't mock Throwable, so we have to test via reflection

70fc400

Spec for a non-serializable object exception

1efdb73

Implementing NonSerializableObject as per specification

b88648b

Strict types declaration

c15beb6

Spec for a TypeError (for when unexpected stuff is passed around)

74c0ec3

Implemented TypeError as per spec

5361896

Removing strict coverage checking, as it prevents static inclusion

a68b09f

When a non-object is passed to NonSerializableObject, an exception …

2fa8fe1

…should be thrown

Throwing when a non-object is passed to NonSerializableObject

324cc5f

Correcting exception test namespace

377372f

Test asset: not serializable class

798a581

Spec for a trait that prevents object serialization

b5b43d8

Implementing NonSerializableObject as per spec

e71c7a8

The __sleep method defined by the Serialize trait must be final

c76550c

Making __sleep final, as per spec

3ec89ba

Spec for an exception produced during attempted de-serialization of a…

947fc62

…n object

Isolating de-serialization prevention in its own exception, for easie…

77e59ed

…r handling/catching

Implementing NonDeSerializableObject as per spec

530cb6a

Corrected method call (was calling non-existing method) - c&p mistake

3f2ba33

Test asset: class that is not de-serializable

0a7ecfa

Spec for a trait that disables de-serialization

854802a

Implemented DeSerialize as per spec

37ad6b0

malukenho suggested changes Oct 11, 2016

View reviewed changes

malukenho reviewed Oct 11, 2016

View reviewed changes

asgrim requested changes Oct 11, 2016

View reviewed changes

asgrim removed their assignment Oct 11, 2016

Ocramius added 4 commits October 11, 2016 12:18

Bumping to PHP 7.1 dependency, because YOLO

8333498

Renaming traits as per @macnibblet's review, since the "don't" is dro…

f5fd3ba

…pped when importing the trait, otherwise

Importing used symbols

8a26ccb

Renaming the library to roave/dont as per @asgrim's review

96e4e2c

asgrim reviewed Oct 11, 2016

View reviewed changes

Renaming DeSerialize to Deserialise, and Serialize to `Serialis…

17bcc62

…e` as per @asgrim and @akrabat review

asgrim approved these changes Oct 11, 2016

View reviewed changes

Ocramius added 5 commits October 11, 2016 12:32

Adding void return signature, since the package is now 7.1+

d68e34b

Adding void return type to all test methods

47470d2

Correcting parameter name as per @malukenho's review

516d488

Travis repo configuration

8d18eb8

Adding humbug base config

f6b85fc

dantleech reviewed Oct 11, 2016

View reviewed changes

Ocramius added 5 commits October 11, 2016 12:46

Requiring humbug/humbug for mutation testing

950a719

Humbug output is to be ignored

2e74b4b

Documenting package purpose

799b66f

Added scrutinizer-ci config

34f9c1b

Adding traditional status badges

7f98f27

Ocramius merged commit f969755 into master Oct 11, 2016

Ocramius deleted the feature/dont-serialize branch October 11, 2016 11:23

Ocramius assigned Ocramius and unassigned malukenho Oct 11, 2016

lexidor mentioned this pull request Jan 7, 2020

[ Naming ] Class and method names don't match the PHP spelling #23

Closed


		use LogicException;

		class NonDeSerializableObject extends LogicException implements ExceptionInterface

Feature: disallow serialization #1

Feature: disallow serialization #1

Conversation

Ocramius commented Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malukenho Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macnibblet commented Oct 11, 2016

Ocramius commented Oct 11, 2016 • edited Loading

macnibblet commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgrim Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgrim Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgrim Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Oct 11, 2016 •

edited

Loading

malukenho Oct 11, 2016 •

edited

Loading

Ocramius commented Oct 11, 2016 •

edited

Loading

asgrim Oct 11, 2016 •

edited

Loading

asgrim Oct 11, 2016 •

edited

Loading

asgrim Oct 11, 2016 •

edited

Loading

Ocramius Oct 11, 2016 •

edited

Loading