Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Spin-off from #1696640: Implement API to unify entity properties and fields. This is just the TypedData portion of that issue's work, presented here on its own, because it is useful for #1648930: Introduce configuration schema and use for translation and #1696302: [meta] Blocks/Layouts everywhere in addition to that issue.
Comment | File | Size | Author |
---|---|---|---|
#18 | typed_data.patch | 51.46 KB | fago |
#13 | typed_data.patch | 48.92 KB | fago |
#7 | typed_data.patch | 44.28 KB | fago |
#6 | typed_data.patch | 32.3 KB | fago |
d8-typed-data.patch | 42.21 KB | effulgentsia | |
Comments
Comment #1
yched CreditAttribution: yched commentedSo i guess the final plan involves moving Field API's hook_field_schema() on top of TypedData ?
That would involve a way to generate a proper db schema out of TypedData info.
Do we plan for this here or leave the question out as a followup ?
Comment #2
Jose Reyero CreditAttribution: Jose Reyero commentedThis looks really cool. I've reimplemented config metadata using this, see https://drupal.org/node/1648930#comment-6459898
Some comments about the TypedData API, issues when putting it to work (and solutions in the patch):
- At some point I've needed some Variant data type to wrap plain data that has no defined type. Is that somehow in the plans? Others like generic 'object' or 'array' may be also useful.
- Had to reimplement, kind of drupal_wrap_data() to handle nicely the cases where the data type is not defined. See ConfigWrapper::buildProperty() in the patch.
- I find StructureInterface has way too many methods and some have barely defined parameters, which is a problem when implementing it. (ConfigWrapper implements it). Specially these:
+ toArray() does not define whether it returns a plain array or a nested array
+ get(), and set() methods are way too generic, cause clashes if you want to implement other interfaces. Suggestion getPropertyValue(), setPropertyValue()
+ setProperties() can take values or properties, which would need some complex logic (specially for cases like these when we need to do complex property rebuilds). Not implemented completely in the patch
Also in this case the wrapped data is some complex array structure. It would help to define property name with dot notation maybe (image.style.effec.etc) when data is nested arrays or data has objects. I guess this may be the case of other objects too, so it would help whether we define when we return a plain array (Key is site.name) or a nested array [site][name]. Just adding to the description "returns a plain array of properties" would help.
Anyway, this looks great and I'm looking forward to TypedData API to be committed (is it this patch or a bigger one?)
Comment #3
chx CreditAttribution: chx commentedYay, type magic! Glad I am here. :D
Couldn't this be somewhere in a class along with drupal_wrap_data as method? Even as a static method? Something :) ?
Any reason not to use __FUNCTION__ here as usual?
I am not in favor of an empty interface. But this is just a personal preference. Even if you keep this there is absolutely no reason to specify IteratorAggregate, you wanted Traversable.
Trivial fix.
See above re IteratorAggregate.
Any resource or a file resource? Both getValue and getString strongly hints at file resource while setValue is no conclusive.
This? In D8??? One setter specifying two different properties depending on their types? Crell used to beat me senseless just for reusing an argument for reset purposes.
In general, compared to the above this makes sense cos value is just different date formats. But the above wasn't about the same data formatted two ways...
Does this need formatting choice be documented somewhere? Moved into a class constant for a potential derived class?
UGH. Aren't decimals fixed point compared to float point? Like, I dunno, the decimal.Decimal type in Python, or the MySQL decimal type or ... everything I ever heard of with this name?
This will be a lot of fun. By demanding is_integer you ban any user input to be fed straight in here. May I recommend (string) (int) $value === (string) $value? (If I missed any other is_integer please check it too. Perhaps is_float too)
Suuuuure. How did we arrive to this wonderfully simple string?
Comment #4
fagoYay, thanks for the great review!
Yep, we've the issue #1753452: Remove dependency on drupal_wrap_data() for it. I've not worked on it yet as moving to data types as plugins would fix it anyway. Also see #1732724: Implement data types as plugins..
Hehe, true having methods for that would be nice and overloading parameters this way is generally a bad thing. However, this "magic" allows the system to act intelligently what is handy sometimes. E.g. if your dates are stored as timestamp you don't have to worry about using the right methods or creating a datetime object yourself, you can just pass in the timestamp and the system will figure it out for you. You can find a table of supported primitives and possible formats for them in the summary of #1696640: Implement API to unify entity properties and fields and #1525958: revisit primitive data types.
Taken to the property API use case it would mean that you'd have to do something like
$entity->created->get('value')->setByTimestamp($timestamp);
instead of just going with
$entity->created->value = $timestamp;
what will call setValue() with $timestamp for you. Thus, I'd argue that this improved DX makes this a valid exception of the do-not-overload-parameters rule.
In general, compared to the above this makes sense cos value is just different date formats. But the above wasn't about the same data formatted two ways...
Oh it should be the same data just differently formatted? Maybe this boils down to a "resources" issue, see following.
Yeah, I guess it should be a file resource, I've not thought of others while writing this. Or other resources that make sense for "binary" data? So we could check for the resource type, but we should keep support for stream wrappers here + clarify the docs on that. Would that address your concern?
Good question. Probably we should add the string representations used to our documentation table.
hm, decimal should not relate to a certain way of storage, but on the data being a number. Floats are PHP's way of representing numbers so using that as our representation is the most logical one. We could just go with "number" but then the PHP representation would be insufficient as it would not cover irrational or complex numbers. We could go with "float" though, but to me it puts more emphasis on the floating point representation whereas we I'd prefer to not emphasis on that or on fixed points. To the semantics it should not matter how that's represented, but maybe in reality it matters anyway and we should just go with "float"?
Anyway, "decimal" does not imply a fixed point representation to me, but I'm not sure how widespread any use of "decimal" for fixed point representations is.
XMLSchema does it that way:
Also see http://www.w3.org/TR/xmlschema-2/#decimal.
Ah thanks, that should be is_numeric() I think - I've fixed it that way already for dates in the latest code. Or is (string) (int) $value === (string) somehow better than is_numeric()?
Unfortunately PHP misses a way to format date intervals in the iso8601 like-way it supports for DateInterval::__construct(). The solution is the nice string. I guess it could need a comment though ;-) - added that.
The empty interface is necessary as it specifies that all these interface must be implemented for lists. We'd need that interface so you can use it to check whether a wrapper is a list..
@traversable: Yeah, I already tried to go with Traversable but failed. Help welcome :-)
Comment #5
fago@traversable:
When I try to do that I get
Fatal error: Class Drupal\Core\Entity\Property\ItemList must implement interface Traversable as part of either Iterator or IteratorAggregate in Unknown on line 0
even when I add implements IteratorAggregate to the class..
Comment #6
fagoHere is an updated patch based upon the entity-property branch in the sandbox.
Comment #7
fagook, this one should be complete. It contains some bogus references on the property item's of the property API though.
Comment #8
Lars Toomre CreditAttribution: Lars Toomre commentedThese are some comments from a first read of this patch focused on documentation and high level review.
Would change s/wrapper. Else/wrapper; otherwise,/
Same as previous comment.
Think it would be clearer to replace 'used class; with something like 'class for type of data'. Used is a confusing adjective here.
I believe @see directives should be in alphabetical order. Repeated in multiple places in patch.
I believe this needs to be Checks rather than Check. Determines might be a better verb in this case.
Suggest 'TRUE if the given user has access; otherwise, FALSE.'
I think documentation calls for blank line between namespace and use. Repeated in multiple places in patch.
Not sure if part of documentation yet, but think use statements should be in alphabetical order (based on other patch reviews).
Again a blank line should be added between namespace and use.
My understanding is @throw directive should appear after @return directive. Repeated elsewhere in patch.
Exceeds 80 characters... Needs to be shortened.
Each of these need a description.
Should be 'if it is not set'.
Needs a short one line description as well as @param and @return directives.
Comment #9
fagoad#2
I've opened #1789066: Add an undefined data type.
I see. This shouldn't be necessary any more once we do #1789066: Add an undefined data type and a wrapper for it.
It says it has to be an array keyed by property name containing the plain property values, so imo this should be clear. Whether the resulting array will be nested depends on your property values so.
Clashes with which one specifically? For a data structure it sounds logical to me that get() gets you a contained property. Anyway, it cannot be getPropertyValue() as it does not get the value, but the wrapper object though. So we could change them to getProperty() and setProperty() if that is preferred - more opinions on that?
True. I'm not sure why the code for it should be so complex though, as it should suffice to loop over your child properties and set the value. Still, given the name one could argue it should only accept property wrappers, so I'd be happy to discuss this further. Looking at the code it seems to be rarely used anyway. Please open an issue for it in the sandbox queue if you are up for it.
Yeah, nested arrays is something we do not specifically support. The map well back to nested data structures though, so I don't think this is something we should add some special support for. See my comment on the other issue on how that could work.
Still, to referring to a property somewhere in a nested structure I think the dot notation or a token like notation makes sense. This patch does not come up with something for that yet, but I at some point something like that will be needed and we should look into coming up with a common "data selection" path syntax and helper for evaluating them on a wrapper.
Comment #10
chx CreditAttribution: chx commentedSo the fundamental difference between the magic handling of file resources and date is that for file resources you store whatever is passed in and do the conversion on get. On the other hand, date converts things immediately, it does not store a number OR a date object. But files do this for performance reasons, right? OK then but it needs a comment explaining that this is a performance trick.
> So we could check for the resource type
No. You can't anyways AFAIK. Just fix the documentation.
> UGH. Aren't decimals fixed point compared to float point? Like, I dunno, the decimal.Decimal type in Python, or the MySQL decimal type or ... everything I ever heard of with this name?
You certainly wanted numeric. That's what PHP uses for "all sorts of things that look like a number".
> Ah thanks, that should be is_numeric() I think - I've fixed it that way already for dates in the latest code. Or is (string) (int) $value === (string) somehow better than is_numeric()?
5.5 is numeric. So is 0x55. Neither of which will work here.
> @traversable: Yeah, I already tried to go with Traversable but failed. Help welcome :-)
Ping me on IRC.
Comment #11
Jose Reyero CreditAttribution: Jose Reyero commented@fago #9
You need to clarify support for that or at least provide some example because my guess is most Drupal objects have some kind of nested properties, like this one (Config Metadata).
Then if we don't support that this StructureInterface doesn't have too much practical use.
(About setProperties)
That's not an answer if you are defining an Interface, thus this is a method we do need to implement. If 'rarely used', please just drop it from the interface.
More:
- get() and set() methods are poorly named and inconsistent (while 'get' returns a WrapperInterface, 'set' returns a plain value.). We'd be better off with getValue(), setValue() and/or getProperty(), setProperty() -- about this last one, never thought of dinamically adding properties to an object?
- It is for nested data structures where get() and set() methods fail too. What do we do when property name is not an end WrapperInterface but an array of them? There are no methods at all to support that unless you give me some WrapperInterface object that is itself a list of properties.
- While on one side I've seen on some other patch a big number of data types added just for entities (like Drupal\Core\Entity\Property\BooleanItem....) thi is failing to support very basic data types like can be a generic array or a generic object for other uses of the API, like this one may need (just because there's not a suitable data type in the base API). This makes me think this is not really suitable for any of them unless really extended. (Should we go for ConfigArray, NestedConfigObject, etc... )?
-- We are not going to need an ...Item type for each data type, are we? Because if we are I think there's something really wrong with the whole approach. I'm talking about #1696640: Implement API to unify entity properties and fields
Really, while basic TypedData/Type/* look useful and well defined to me, the StructureInterface (and all the others in the pack) don't look so. An interface should have the very minimum methods you need just because we are going to
implement all of them every time we use it.
Given all these issues, while using basic TypedData seems the way to go (at least if we can get ASAP some undefined data type), waiting fo the whole thing to get committed will be a major roadblock for Config Metadata, and thus for localized configuration (In other words, I don't see it happening in time for feature freeze giving us some margin to build on top of it).
So really either we simplify, stick to the very minimum, and move on with this and get it committed ASAP or we are going to need some other solution for the configuration metadata (note we are really targetting localized configuration here and that means either we get some time after this gets committed to build on top of that or this will be just useless for other purposes).
An interim solution for config metadata may be reusing only the basic data type names (i.e. 'string') and then let using full TypedData classes for the future if possible...
Comment #12
fagoAs said, we do not not need specific support as it works as is, i.e. entities itself are nested data structures consisting of three levels. As described at http://drupal.org/node/1648930#comment-6495492:
I'm happy to guide you through this on IRC if you like.
Get and set both work with wrappers. set does not returny anything?
You can do that, just have getPropertyDefinitions() dynamically return the definitions.
If you want an entity property for it, yes. It's already the same way for fields, i.e. you need to create a field type "integer" for an interger... What's wrong with that?
Yeah, there was no need for generic array/object or list yet. If there is need for that we should look at adding it.
Anyway, the whole patch over at #1696640: Implement API to unify entity properties and fields is close to ready and heavily worked on. I don't think there is or will be a timing problem.
Comment #13
fagoad #8: Thanks, I've incorporated most of the review. I've not done the @see re-ordering yet as the current ordering at least partially makes sense, but if there is style guide to order it I'm happy to. Any pointer?
>Not sure if part of documentation yet, but think use statements should be in alphabetical order (based on other patch reviews).
I've mostly done so but in a meaningful way, e.g. left exceptions at the end. The same here - I'm happy to follow what's specified in the guidelines but I think those are still in flux.
ad #10: thanks, I've fixed all that.
@setProperties from #11: Yep, meanwhile I've worked on that - see #1794132: Clarify differences between setValue() and setProperties(). Now setProperties() is gone in favour of setPropertyValues(). Updated patch attached that mostly extracts the typed data API parts. Note that the updated patch also contains the conversion to the plugin system, see #1732724: Implement data types as plugins..
Comment #15
chx CreditAttribution: chx commentedI thought we agreed on renaming this to numeric. Didn't we? Or if it's a float, then to float. Decimal is a non-starter because decimal is fixed point, everywhere, I showed examples. Because you linked http://www.w3.org/TR/xmlschema-2/#decimal here it is:
vs float
. See below for more.
I know setValue has this commented but I think that comment is needed here as well.
So we are fine storing NULL into $this->value? Why...? Shouldn't even $this->value start at , say, FALSE? If it is a boolean, then it is a boolean :)
Isn't this unnecessary , isn't this coming from the TypedData class?
Well, see above: decimals can't be represented by floats.
Again , what's our feeling about NULL? Always possible to get it?
The is_numeric is superflous, the "super int" checker will take care of that. Also, this (and the other copy of it) needs a comment "Allow any integer regardless whether it's supplied as an integer or a string."
Ouch! langcode is typically a string like 'es'. By making it into a TypedData object, some major confusion will abound. Why this can't be $value like every other?
????? we do not have ER in core.
Comment #16
fagoYeah, NULL is always valid, meaning there is no value set - what is different to FALSE. That's actually quite important for use to differentiate as it's the only way to figure out that something is not set and thus doesn't need to be stored.
Then, having the typed data object wrapping NULL gives a better DX then having NULL instead of the object. This saves us from having to do quite some checks ;)
True. My feeling was that this might be more efficient than doing quite some casts with e.g. DateTime objects just for checking. What do you think, or know?
This helps a lot when this is used as computed property, which lazy loads the language object based upon the language code. By keeping a reference on the language code object we always stay in sync with potential updates.
As mentioned this is not 100% separated from #1696640: Implement API to unify entity properties and fields. So the patch over there adds this for storing entity references like the node author.
hm, as said decimal and float are certain represntations. While the data definitions is about the semantics of the data, it's part of the real world and limited to certain representations. So, I guess there is no native way to represent decimals in PHP then? Still, the decimal representation could come from the storage what is fine as long as PHP is able to respresent it (as float?).
So we could label this "float" and put emphasis on its PHP representation or label it "numeric" to put emphasis on not being limited to a certain representation - what's not the case though. Looking at others (XMLSchema, PHPCR, SDL) all have both decimal and float/double so I think we should follow that: Rename our decimal to "float" and add a separate decimal primitive.
Comment #17
chx CreditAttribution: chx commented> My feeling was that this might be more efficient than doing quite some casts with e.g. DateTime objects just for checking. <= pointless microoptimization. Are you really worried about the speed of three typecasts??
> Ouch! langcode is typically a string like 'es'. By making it into a TypedData object, some major confusion will abound. Why this can't be $value like every other?
Just don't call it $langcode and we are good.
Let's call it float and let's not add decimal later. To implement decimals with a value of i * 10 ^ -n where i and n are both nonnegative integers you would need to reimplement arithmetic with those cos PHP doesnt support them... or store them as strings which is a farce cos when you try to do arithmetic they are converted into floats.
Comment #18
fagook, implemented all three points.
Comment #19
fagooh this patch misses service registration so tests will fail. Anyway, it should still help reviewing the typed data API.
Comment #21
chx CreditAttribution: chx commentedI think this is at least a major bug now that the generic set-get brought TypedData in without these fixes in here.
Comment #22
Jose Reyero CreditAttribution: Jose Reyero commentedI'm having some trouble with an Interface extending both ComplexDataInterface and ListInterface. Not sure whether I am doing sth wrong or it is an issue with these interfaces, they are incompatible or what...?
Please see https://drupal.org/node/1648930#comment-6538230
Comment #23
fagoAll fixed discussed here got implemented and commited as part of #1696640: Implement API to unify entity properties and fields, at least as far as I know. Thus I'm marking this as fixed, please re-open if you think something has been left out.
Yes. Can something be a list and complex data at the same time? I don't think so, but of course it'd be happy to discuss that. Feel free to ping me in IRC/skype/.. or open issue for it.