Last modified: 2014-05-16 20:42:15 UTC
As of the introduction of the Fingerprint class, the setLabel, setDescription etc functions are now deprecated. This means that the preferred way to set a label is now: $fingerprint = $item->getFingerprint(); $fingerprint->setLable( new Term( 'en', 'Foo' ) ); $item->setFingerprint( $fingerprint ); Compare top the previous: $item->setLabel( 'en', 'Foo' ); That's quite verbose and hard to read, and it's even unclear/undocumented whether the setFingerprint is needed, or whether manipulating the Fingerprint object returned by getFingerprint is sufficient. I suggest to restore the convenience functions at least for adding/setting/removing labels, descriptions, and aliases. They should just wrap the respective functionality in Fingerprint. The situation for getting a label or a list of aliases has also become more complicated, and should probably be improved: $label = $item->getFingerprint()->getLabel( 'en' )->getText(); $aliases = $item->getFingerprint()->getAliasGroup( 'en' )->getAliases(); instead of before: $label = $item->getLabel( 'en' ); $aliases = $item->getAliases( 'en' ); I see the benefit of using composition for the fingerprint; I do not see the benefit of deprecating the convenience functions. The new way just makes reading harder by adding redundant noise.
I created a discussion item for this a few weeks ago and started collecting oddities: https://github.com/wmde/WikibaseDataModel/issues/73 Now that we are actually working with the Fingerprint classes in the ChangeOps it turns out to be as unpleasant as I was afraid of. Sorry. This is really not meant to be personal. The initial attempt was nice and well-meant: Use objects for everything. Make most immutable. This sounds nice from a design perspective but makes doing real stuff (like checking if changing a label is going to work before actually changing it, like it is done in the ChangeOps) complicated. There are multiple problems: The naming conventions inside the Fingerprint classes are just not consistent and self-explaining. For example, methods are using the word "descriptions" inconsistently. It can mean anything, an array of strings, an array of Description objects, an array of Term objects or a List object (what I described as "reimplementing arrays" before). Some methods that are named after classes return strings and string arrays while others that sound like they return strings return objects. See https://github.com/wmde/WikibaseDataModel/pull/86 for a discussion about the naming convention. The currently most unpleasant use case is when a ChangeOp wants to check if modifying a Fingerprint is going to work without actually changing the object. It needs a copy. But how to do this? If you want to avoid PHP's clone you need to create a very complicated method that iterates all labels, terms and aliases in all alias groups, avoids re-using any of the nested objects and instantiates new objects from the values. There are methods to get string arrays of labels and descriptions but none to create objects from such arrays. The most straightforward interface would, in my opinion, look like this: class Fingerprint { string[] getLabels(); setLabels( string[] ); string[] getDescriptions(); setDescriptions( string[] ); array[] getAliases(); setAliases( array[] ); string getLabelForLanguage( string ); setLabelForLanguage( string, string ); string getDescriptionForLanguage( string ); setDescriptionForLanguage( string, string ); string[] getAliasesForLanguage( string ); setAliasesForLanguage( string, string[] ); } All arrays are associative arrays with the language codes as keys. Internally the class can still use Term objects, List objects and so on. If the interface is clean and easy to use that doesn't really matter in the end. An other attempt is to make the Fingerprint classes partially do what is currently done in the ChangeOp validation. Example: $fingerprint->setLabel( '' ) raises an exception if a hard constraint is validated. Then we don't need any methods to de- and reconstruct a Fingerprint from the outside. But I'm pretty sure that's not the way we want to go.
