[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
Ladsgroup added a comment. I did some parts (subtasks) but I shouldn't get credit for all of itTASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: LadsgroupCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
Ladsgroup added a comment. That and updating https://github.com/wikimedia/mediawiki-extensions-Wikibase/blob/master/docs/extending-entities.wiki I'll make a patch in secTASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: LadsgroupCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
gerritbot added a comment. Change 328357 merged by jenkins-bot: Use ChangeOpDeserializer callbacks in Api\EditEntity https://gerrit.wikimedia.org/r/328357TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, gerritbotCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
gerritbot added a comment. Change 331609 merged by jenkins-bot: More generic "@return ChangeOp" type for EditEntity::getChangeOps https://gerrit.wikimedia.org/r/331609TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, gerritbotCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
gerritbot added a comment. Change 331609 had a related patch set uploaded (by Thiemo Mättig (WMDE)): More generic "@return ChangeOp" type for EditEntity::getChangeOps https://gerrit.wikimedia.org/r/331609TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, gerritbotCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
daniel added a comment. In T152491#2923592, @Aleksey_WMDE wrote: If we need to wrap it in a closure I don't understand why do we need an interface in the first place? The callback is the factory for the actual deserializer object. The entity type definition file functions as a DI wiring file. It is of course possible to use the callbacks directly. And if PHP has function types, that's what I would suggest doing. But since PHP doesn't have that, it's nicer to have an interface and objects, to provide at least some type safety.TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, danielCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
Ladsgroup added a comment. AFAIK it's the plan to wrap it up in a closure (it'll be added here)TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: LadsgroupCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
Aleksey_WMDE added a comment. As soon as we don't need to wrap it in a closure I don't see a reason to put callback in the name - it will only bring confusion. If we need to wrap it in a closure I don't understand why do we need an interface in the first place?TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, Aleksey_WMDECc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
WMDE-leszek added a comment. The more important question I think I failed to make more promiment is whether entity type definition should be providing the ChangeOpDeserializer instance or just some generic callback returning ChangeOp instance. Former seems better to me, regardless how the actual property/field in the type definition will be called. Having callback for consistency seems reasonable IMO.TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, WMDE-leszekCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
daniel added a comment. @Aleksey it seems a bit redundant here, but we use it for other callbacks in the entity type definition. I vote to keep in in the name of consistency. We could make it optional for all fields in the entity type definition. But let's have a separate discussion about that.TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, danielCc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
Aleksey_WMDE added a comment. Vote for dropping "callback" suffix.TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, Aleksey_WMDECc: thiemowmde, Aleksey_WMDE, gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
daniel added a comment. @Ladsgroup I had a quick look at your patch, and it seems a good first step. It would be nice to do some refactoring to make this a bit cleaner, but it should work as it is. Too bad PHP's type system doesn't support function signatures, that would remove the need for classes/interfaces to make this kind of thing type safe. Anyway, I'll have a closer look later. Perhaps make a patch in which MediaInfo uses the new mechanism, so we can see this in operation.TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, danielCc: gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
Ladsgroup added a comment. I made https://gerrit.wikimedia.org/r/#/c/328357 before your comment @daniel (I forgot to put Bug: T### properly in the commit message), but it's practically the same. Please give me your feedback and we continue based on your feedbackTASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: LadsgroupCc: gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, Th3d3v1ls, Ramalepe, Liugev6, Lewizho99, Maathavan, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
gerritbot added a comment. Change 328357 had a related patch set uploaded (by Ladsgroup): Make EditEntity injectable for getChangeOps https://gerrit.wikimedia.org/r/328357TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, gerritbotCc: gerritbot, Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
daniel added a comment. Yes, as Leszek said: hooks should be avoided whenever possible. They are easy to introduce, but they rely on global state. Because of that, they hide information flow, and make testing hard. So, EditEntity::getChangeOps should use a callback defined in the entity type definition to construct ChangeOps from JSON. The current hard-coded logic in EditEntity should be moved accordingly. I don't think all of that should go into a callback, we'll probably want something like a ChangeOpDeserializer interface (think of the JSON as serialized ChangeOps). The callback would return a list ChangeOpDeserializer appropriate for a given entity type. Ideally, we wouldn't inject an untyped callback, but we'd have a ChangeOpDeserializerFactory interface... but that may be overkill. The above model is just off the top of my head. There may be better ways. There is already a ChangeOpFactoryProvider and several XyzChangeOpFactory classes that will have to fit into this somehow. The important factors are: EditEntity should not know how to construct ChangeOps the entity type definition array should provide a callback that somehow provides the knowledge of how to construct a list of ChangeOps from the JSON input to EditEntity. that callback (or something that wraps it, or something that was constructed by it) needs to be injected into EditEntity. Note that EditEntity currently uses pseudo-injection by accessing global state in the constructor, and it does not provide an overrideServices method. Changing this to using proper injection would also be nice. To achieve this, the API module can be registered using a callback instead of a class name. When working on this, please file the appropriate tickets first.TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Ladsgroup, danielCc: Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
WMDE-leszek added a comment. I haven't looked at the problem in detail yet but I am sorry to say it seems to me that having some kind of definition in entitytypes is a better option here. Using hooks seems a bit hacky here, while entity type definitions exist for dealing with such issues as I got it. I can imagine this is not going to be an easy task to change it all to work, that's a shame. I talked briefly with @daniel about this and he also doesn't like the hook option much. I guess he will chime in here with some more comments soon.TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: WMDE-leszekCc: Jakob_WMDE, WMDE-leszek, Aklapper, Addshore, Ladsgroup, daniel, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T152491: Allow the EditEntity API module to work with all types of entities
Ladsgroup added a comment. Okay, I checked a little, It seems we have two options: Register a hook: We can add a hook to end of Wikibase\Repo\Api\EditEntity::getChangeOps so WikibaseLexeme can patch and send it up. We can add a key to entitytypes array called ApiPatchCallbacks and make Wikibase\Repo\Api\EditEntity::getChangeOps use these patching callbacks. The former is easy to implement but we might run into trouble if two wikibase extensions use the hook and try to patch same things (e.g. we have two wikibase extensions that add lemmas). The latter is probably a royal pain.TASK DETAILhttps://phabricator.wikimedia.org/T152491EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: LadsgroupCc: Aklapper, Addshore, Ladsgroup, daniel, D3r1ck01, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
