Self serializing nodes

[spawnia]

Rearchitect the way nodes are serialized and align it more closely with Slate.

The functionality is the same as before, but the API is changed quite a bit.

The logic and rules for what each individual node may contain are now
contained within each node, instead of the monolithic Unserializer class.

This allows deserialization at all levels of the object model.

Open to discussion if you think that changes are necessary :)

[e1himself]

Hi @spawnia!

We're grateful for your effort to make this library better 👍
That's a great spirit!

However, this pull request is a huge dramatic change with multiple Backwards Incompatible changes.
I'd say it's better to name it a re-write of this library.

To summarize, here's a list of BC-breaking changes of this PR:

• All the getters are now replaced by public properties (BC break)
• Entity interface has been dropped (BC break)
• Node interface has been dropped (BC break)
• Unserializer has been dropped (BC break)

---

Also you've made a number of style changes that are not aligned with this repository's code-style.

• Our intention was to keep classes hierarchy domain-model-driven, not implementation-driven. That's why we think we should keep the Node and Entity interfaces in the hierarchy, but there should not be JsonConvertible or TextContainingNode parent classes.
• Our test methods are intentionally following the it_should_ convention. We don't want it go.

---

That being said we cannot accept this pull request. Sorry.

What was the reason that you've decided to do such a dramatic rewrite? Do you have problems using the existing API in your project?

[spawnia]

Thanks for your reply @e1himself

I agree with your assessment concerning the scope of this PR, still i think the changes are worthwhile.

The main reason for the changes is to align this implementation more closely with the reference implementation. The models there work similar to what i changed them to.

All the getters are now replaced by public properties (BC break)

That makes programmatic manipulation of the nodes much easier.

$leaf = new Leaf('foo', [new Mark('type')]);

// Modify text
// Before
$leaf->setText(
	str_replace($leaf->getText(), 'o', 'a')
)
// After
str_replace($leaf->text, 'o', 'a')

// Modify children
// Before: Not possible atm, can only add. Given we added set:
$marks = $leaf->getMarks()
$leaf->setMarks(
	array_map(
		$marks,
		function(Mark $mark): Mark {
			// modify mark
			// return modified mark
		}
	)
);
// After
foreach($leaf->marks as $mark){
	// modify mark
}

Static analysis can catch a misuse of those properties easily, and typed properties in PHP 7.4 will make it even safer, so i do not see a strong argument for using getters/setters.

Entity interface has been dropped (BC break)

I did not like how this interface was "all-knowing", holding all the constants of its children. In the other direction, all children depended on specific constants in there.

Node interface has been dropped (BC break)

I replaced that with the TextContainingNode abstract class which is both more descriptive
and brings this implementation more functionally close to the JS implementation, where "text" is a computed property.

Unserializer has been dropped (BC break)

This also makes the API simpler and more powerful:

// Unserializing a value
// Before
$unserializer = new Unserializer();
$unserializer->fromJSON('{}');
// After
Value::fromJSON('{}');

// Unserializing anything else
// Before: not possible
// After
Mark::fromJSON('{}');

Our test methods are intentionally following the it_should_ convention. We don't want it go.

Well the idiomatic PHPUnit way is to begin methods with test. I have been annoyed many times when i forgot a @test annotation, so choosing this convention seems simpler to me.

If you really want it reverted, that is fine with me. I can change the PR if that is all that is holding it back.

I hope i could clear up the intentions behind my changes. I am open to shaping this PR to a point where we can both agree on. It will not be possible without breaking changes though.

[e1himself]

@spawnia

1. I'm still struggling to understand what problem does this PR solve.

   // Unserializing anything else
   // Before: not possible
   // After
   Mark::fromJSON('{}');

   Why do you need this? We're using Slate content in our PHP backend quite heavily. We do a lot of mapping/filtering/modifications (the manipulating code is not a part of this repository though). But I don't see why one need to deserialize a mark from JSON.

   UPD: I've created an issue to discuss this: Not possible to deserialize a subset of Slate value #18

2. The public properties approach

   That makes programmatic manipulation of the nodes much easier.

   You're right that the manipulation can be easier. But I'm not sure that it is the approach we want to simplify.

   I'd better embrace the immutable approach for the long-term roadmap. When you cannot modify an existing model instance, only create a new one with updated properties.

   That will be much closer to the spirit of Slate JS.

   Something like this:

   $leaf = new Leaf('foo', [new Mark('type')]);
   // Modify text
   $leaf = $leaf->withText(
       str_replace($leaf->getText(), 'o', 'a')
   );
   // Modify children
   $leaf = $leaf->withMarks(
       array_map(
           $leaf->getMarks(),
           function(Mark $mark): Mark {
               // modify mark
               // return modified mark
           }
       )
   );

   UPD: I've created an issue to discuss the Immutability approach: Embrace immutability #19

3. Oh, that's a problem that should be fixed.

   Before: Not possible atm, can only add.

   I've added an issue for it: Not possible to set Leaf marks, only get or add #17