[DNM] Make factories support custom entity types #212

thiemowmde · 2017-02-09T12:13:22Z

Do not merge! I figured this is the wrong approach. See https://gerrit.wikimedia.org/r/338749 instead.

~~This is more than a convenient method, but solves an actual issue. This allows us to construct a SerializerFactory (and pass it around) that is able to serialize more than the basic entity types.~~

Please review this along with wmde/WikibaseInternalSerialization#110. Both changes must be done together, or not. As @JeroenDeDauw hinted below the architecture proposed in these two patches might be flawed. Could it be that we are just confused by these duplicate (De)SerializerFactories? That other component is called "internal". What does this mean? Could it be that the only thing we need to do is to rename these components and classes to make it more obvious what they represent?

Bug: T157596
Bug: T157960

manicki

No tests for the new method.
I reckon this method would provide a serializer that could handle e.g. items, properties and entities of type X but it there still would not be a way to get a serializer fo entity of type X (I am not sure though if we need such thing, and if this is what you meant by "does not solve a pressing issue").

manicki · 2017-02-09T12:22:29Z

src/SerializerFactory.php

+	 *
+	 * @param DispatchableSerializer[]|callable[] $dispatchableSerializers A list of either
+	 *  DispatchableSerializer objects, or callbacks that return such objects.
+	 *


manicki · 2017-02-09T12:23:08Z

src/SerializerFactory.php

+				$serializer = call_user_func( $serializer, $this );
+			}
+
+			if ( !( $serializer instanceof DispatchableSerializer ) ) {


Shouldn't there be some message?

manicki

I see no major issues with the production-code side of the PR. Usage of SerializerFactory still needs to adjusted (obviosuly), plus SerializerFactory tests should test pasing in the serializer for the entity type using both the actual object, and the instantiating callback.

manicki · 2017-02-10T11:34:33Z

src/SerializerFactory.php

-	 *
-	 * @return Serializer
+	 * @throws IllegalValueException
+	 * @return Serializer A dispatching serializer that can serialize whatever the entity


Shouldn't this then say @return DispatchingSerializer? Or is it only for compliance with the interface?

(nitpick) Also possibly @return could come before @throws, I think this is the order we often have those.

DispatchingSerializer is not private and we could expose it here. But there is not much value in this. The only difference is that this makes the addSerializer method visible. This should not be needed.

I'm always putting @throws before @return, because it reflects the actual control flow: code can either throw exceptions for different reasons, or return something in the end.

manicki · 2017-02-10T11:41:57Z

src/SerializerFactory.php

+	public function __construct(
+		array $entitySerializers,
+		Serializer $dataValueSerializer,
+		$options = 0


As suggested in the other patch, this could also default to self::OPTION_DEFAULT so that users don't have to wonder what is this zero about.

thiemowmde · 2017-02-10T13:22:57Z

I continued working on this and believe this is finished now. The only detail still missing is a specific test scenario for the new constructor parameter.

I also learned ~~that the architecture I build here does have a flaw~~: you need a (De)SerializerFactory to create an array with an Item and Property(De)Serializer, which is needed in the constructor of the (De)SerializerFactory. In other words: you need two factories.

~~Possible consequence: drop the support to pass in (De)Serializer objects and only support callables. What do you think?~~

brightbyte · 2017-02-10T11:49:59Z

src/SerializerFactory.php

+	 *
+	 * @param DispatchableSerializer[]|callable[] $entitySerializers A list of either
+	 *  DispatchableSerializer objects that are able to serialize Entities, or callbacks that return
+	 *  such objects.


This should mention the expected signature of the callback.

brightbyte · 2017-02-10T11:50:39Z

src/SerializerFactory.php

 			throw new InvalidArgumentException( '$options must be an integer' );
 		}

+		$this->entitySerializers = $entitySerializers;


Would be nice to check using Assert::parameterElementType. Depending on Assert should not be a problem, right?

There is not much value in doing such sanity checks. They are already done in newEntitySerializer, and there is no way around this, because we don't want to run the callables on construction time just to check what they return.

I believe @brightbyte meant checking all elements in $entitySerializers are callables (or in the current form, either callable or DispatchableSerializer). This would be OK to check I think (so you can filter out garbage already in the constructor).
Running a callback to check what is the type of its return value would be quite overthinking the whole problem, I agree.

brightbyte · 2017-02-10T11:51:34Z

src/SerializerFactory.php

+				$serializer = call_user_func( $serializer, $this );
+			}
+
+			if ( !( $serializer instanceof DispatchableSerializer ) ) {


Could use Assert::postcondition

brightbyte · 2017-02-10T11:52:08Z

src/SerializerFactory.php

+	/**
+	 * @since 3.0
+	 *
+	 * @param DispatchableSerializer[]|callable[] $entitySerializers A list of either


Why accept both? Do we have a use case for providing DispatchingSerializer objects here?

brightbyte · 2017-02-10T15:22:33Z

@thiemowmde wrote:

Possible consequence: drop the support to pass in (De)Serializer objects and only support callables. What do you think?

Yes, just support callables.

brightbyte · 2017-02-10T17:09:36Z

src/SerializerFactory.php

+	public function __construct(
+		array $entitySerializers,
+		Serializer $dataValueSerializer,
+		$options = self::OPTION_DEFAULT


I'm not quite sure that $options should be in the constructor here. May also be a parameter of newEntitySerializer.

This is not possible. All kinds of public methods (e.g. newTermListSerializer) use these options.

I'm not sure if this is what @brightbyte meant but you could have optional $options parameters in each newXyzSerializer method (or at least in those that make use of options), instead of setting $options in the constructor. I am not sure how these options are actually used (ie. at what point it makes more sense to specify options) so I have no opinion on this.

brightbyte · 2017-02-10T17:38:06Z

"Add SerializerFactory::newDispatchingSerializer" is no longer true, as far as I can tell.

thiemowmde

I minimized this patch a bit. But I can not approve it because of @manicki.

I decided to not remove the feature I talked about earlier: I believe it makes sense to allow the $entity(De)Serializers to contain (de)serializers instead of callables. This can be helpful in situations where a (de)serializer is so trivial that it does not need the factory to construct itself. Or in other words: I think this feature makes sense and is nice. Code-wise it does not make a difference anyway.

thiemowmde · 2017-02-13T11:52:41Z

src/SerializerFactory.php

+	public function __construct(
+		array $entitySerializers,
+		Serializer $dataValueSerializer,
+		$options = self::OPTION_DEFAULT


This is not possible. All kinds of public methods (e.g. newTermListSerializer) use these options.

thiemowmde · 2017-02-13T11:54:24Z

src/SerializerFactory.php

 			throw new InvalidArgumentException( '$options must be an integer' );
 		}

+		$this->entitySerializers = $entitySerializers;


There is not much value in doing such sanity checks. They are already done in newEntitySerializer, and there is no way around this, because we don't want to run the callables on construction time just to check what they return.

manicki

I minimized this patch a bit. But I can not approve it because of @manicki.

Sorry for sleeping on this.
But wait, did we just catch you on attempting to self-merge your PR :)

I had a look at the current state of the patch and it looks quite OK to me. Please just note my question about the exception, I might be asking about something irrelevant there.
Few things @brightbyte suggested are still open but possibly those could be further improvements.

About this:

I decided to not remove the feature I talked about earlier: I believe it makes sense to allow the $entity(De)Serializers to contain (de)serializers instead of callables. This can be helpful in situations where a (de)serializer is so trivial that it does not need the factory to construct itself. Or in other words: I think this feature makes sense and is nice. Code-wise it does not make a difference anyway.

I agree this sounds like a nice feature but then: do we have a real case where this is/could be used? Be it a nice feature but without this being used (if not now than in a foreseeable future) this seems YAGNI to me. I am not strongly opposing this, and I'd be OK with having this, I am just wondering whether something being nice is a good enough justification.

manicki · 2017-02-13T12:54:46Z

src/DeserializerFactory.php

+			}
+
+			if ( !( $deserializer instanceof DispatchableDeserializer ) ) {
+				throw new IllegalValueException(


I wonder about the exception choice here. Isn't IllegalValueException a DataValues-specific exception. How those serializers are related to DataValues? Why not throw standard InvalidArgumentException?

manicki · 2017-02-13T12:56:40Z

src/SerializerFactory.php

 			throw new InvalidArgumentException( '$options must be an integer' );
 		}

+		$this->entitySerializers = $entitySerializers;


I believe @brightbyte meant checking all elements in $entitySerializers are callables (or in the current form, either callable or DispatchableSerializer). This would be OK to check I think (so you can filter out garbage already in the constructor).
Running a callback to check what is the type of its return value would be quite overthinking the whole problem, I agree.

manicki · 2017-02-13T13:01:51Z

src/SerializerFactory.php

+	public function __construct(
+		array $entitySerializers,
+		Serializer $dataValueSerializer,
+		$options = self::OPTION_DEFAULT


I'm not sure if this is what @brightbyte meant but you could have optional $options parameters in each newXyzSerializer method (or at least in those that make use of options), instead of setting $options in the constructor. I am not sure how these options are actually used (ie. at what point it makes more sense to specify options) so I have no opinion on this.

thiemowmde · 2017-02-13T15:56:22Z

did we just catch you on attempting to self-merge

Approval is not self-merge. ;-)

do we have a real case where this is/could be used?

Not at the moment. But this will happen when an entity type does not reuse any of the concepts available in the (De)SerializerFactory.

YAGNI

Look at the code. Disallowing (de)serializer objects and enforcing callables would make it more complicated.

IllegalValueException […] InvalidArgumentException

That was a mistake, good catch! Should be UnexpectedValueException. InvalidArgumentException does not fit because the method in question doesn't even have arguments.

you can filter out garbage already in the constructor

Sure, but as said, I believe there is not much value in this. This is all checked anyway a few milliseconds later.

have optional $options parameters in each newXyzSerializer method (or at least in those that make use of options)

First, I think this is ugly. Second, this would make the return values incompatible to each other. You can no longer pass a SerializerFactory around that is able to construct all kinds of serializers that are compatible with each other, without also passing the $options around with the factory as a separate parameter. In other words: Moving the options to the new… methods means the class would become a factory that can construct all kinds of serialization formats, but I think a single factory should only support a single serialization format. I believe this is less error-prone.

thiemowmde · 2017-02-13T16:56:48Z

Does not matter. Can be replaced with []

I don't know why you say that. I guess you did not run all tests when you tried this. Please check again and be more specific with your edit request.

bekh6ex · 2017-02-13T20:35:18Z

I checked out the code and run all the tests. They don't fail if it is replaced with []

JeroenDeDauw · 2017-02-14T07:51:35Z

Can't really review this one... part of a bigger approach which I think is fundamentally flawed (and counterproductive). Will bring that up again soonish at a better place. I'm a bit sadened no one else is seeing this

thiemowmde · 2017-02-14T09:32:16Z

I […] run all the tests. They don't fail if it is replaced with []

You said that already. As I said you must have done something wrong. When I run the tests they fail when there are no entity (de)serializers. Which makes sense because the tests in question are actually testing the entity (de)serialization.

part of a bigger approach which I think is fundamentally flawed (and counterproductive). […] I'm a bit sadened no one else is seeing this

Do you realize your comment does not give the slightest hint at what you mean? Can you please clarify?

manicki · 2017-02-14T09:40:07Z

Will bring that up again soonish at a better place.

Do you realize your comment does not give the slightest hint at what you mean? Can you please clarify?

I don't think @JeroenDeDauw thinks it does not make sense to explain this here, and I believe it is fine as he thinks the whole approach is wrong, and want to make a more general remark, not just one details of this particular PR.

I am patiently looking forward to what he is going say in that better place :)

It is not impossible that the approach is flawed. We're not going to have a code with this approach released in this very moment, so discussing it and fixing possible improvements seems like a very good idea to me.

bekh6ex · 2017-02-16T17:20:19Z

I checked out the code and run all the tests. They don't fail if it is replaced with []

Sorry, my mistake. Was running wrong set of tests.

thiemowmde · 2017-03-03T13:03:54Z

Wrong approach, this component is the base DataModel serialization, and not supposed to know about other entity types. See https://gerrit.wikimedia.org/r/338749 instead.

thiemowmde added the enhancement label Feb 9, 2017

thiemowmde added this to the 2.3.0 milestone Feb 9, 2017

thiemowmde requested a review from brightbyte February 9, 2017 12:13

manicki requested changes Feb 9, 2017

View reviewed changes

thiemowmde force-pushed the newDispatchingSerializer branch from ef7ce2c to 4fa0512 Compare February 9, 2017 13:50

manicki requested changes Feb 10, 2017

View reviewed changes

thiemowmde force-pushed the newDispatchingSerializer branch 3 times, most recently from ccc73d3 to 53b765a Compare February 10, 2017 13:16

brightbyte reviewed Feb 10, 2017

View reviewed changes

thiemowmde mentioned this pull request Feb 13, 2017

Deprecate newEntity(De)Serializer in factories #188

Closed

thiemowmde changed the title ~~Add SerializerFactory::newDispatchingSerializer~~ Make factories support custom entity types Feb 13, 2017

thiemowmde modified the milestones: 3.0.0, 2.3.0 Feb 13, 2017

thiemowmde mentioned this pull request Feb 13, 2017

Release 2.3.0 #213

Merged

thiemowmde force-pushed the newDispatchingSerializer branch from 53b765a to a4eae04 Compare February 13, 2017 11:51

thiemowmde commented Feb 13, 2017

View reviewed changes

thiemowmde added the breaking label Feb 13, 2017

manicki approved these changes Feb 13, 2017

View reviewed changes

thiemowmde force-pushed the newDispatchingSerializer branch from a4eae04 to 15fbf8d Compare February 13, 2017 15:59

thiemowmde force-pushed the newDispatchingSerializer branch from 15fbf8d to 93b1a10 Compare February 16, 2017 10:59

Make factories support custom entity types

6ebd9db

thiemowmde force-pushed the newDispatchingSerializer branch from 93b1a10 to 6ebd9db Compare February 16, 2017 17:23

thiemowmde changed the title ~~Make factories support custom entity types~~ [DNM] Make factories support custom entity types Feb 17, 2017

thiemowmde removed this from the 3.0.0 milestone Feb 20, 2017

thiemowmde closed this Mar 3, 2017

thiemowmde deleted the newDispatchingSerializer branch March 3, 2017 13:03

thiemowmde mentioned this pull request Mar 6, 2017

Release 3.0.0 #214

Closed

[DNM] Make factories support custom entity types #212

[DNM] Make factories support custom entity types #212

Uh oh!

Conversation

thiemowmde commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manicki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manicki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiemowmde commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brightbyte commented Feb 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brightbyte commented Feb 10, 2017

Uh oh!

thiemowmde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manicki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiemowmde commented Feb 13, 2017

Uh oh!

thiemowmde commented Feb 13, 2017

Uh oh!

bekh6ex commented Feb 13, 2017 via email • edited by thiemowmde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeroenDeDauw commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thiemowmde commented Feb 14, 2017

Uh oh!

manicki commented Feb 14, 2017

Uh oh!

thiemowmde commented Feb 9, 2017 •

edited

Loading

thiemowmde commented Feb 10, 2017 •

edited

Loading

bekh6ex commented Feb 13, 2017 via email •

edited by thiemowmde

Loading

JeroenDeDauw commented Feb 14, 2017 •

edited

Loading