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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

So 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 ?

Jose Reyero’s picture

This 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?)

chx’s picture

Status: Needs review » Needs work

Yay, type magic! Glad I am here. :D

+++ b/core/includes/common.incundefined
@@ -7286,6 +7286,84 @@ function drupal_get_updaters() {
+function drupal_get_data_type_info() {

Couldn't this be somewhere in a class along with drupal_wrap_data as method? Even as a static method? Something :) ?

+++ b/core/includes/common.incundefined
@@ -7286,6 +7286,84 @@ function drupal_get_updaters() {
+  $items = &drupal_static('data_type_info');

Any reason not to use __FUNCTION__ here as usual?

+++ b/core/lib/Drupal/Core/TypedData/ListInterface.phpundefined
@@ -0,0 +1,16 @@
+interface ListInterface extends ArrayAccess, IteratorAggregate , Countable { }

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.

+++ b/core/lib/Drupal/Core/TypedData/ListInterface.phpundefined
@@ -0,0 +1,16 @@
\ No newline at end of file

Trivial fix.

+++ b/core/lib/Drupal/Core/TypedData/StructureInterface.phpundefined
@@ -0,0 +1,102 @@
+interface StructureInterface extends IteratorAggregate  {

See above re IteratorAggregate.

+++ b/core/lib/Drupal/Core/TypedData/Type/Binary.phpundefined
@@ -0,0 +1,81 @@
+ * The plain value of binary data is a PHP resource, see

Any resource or a file resource? Both getValue and getString strongly hints at file resource while setValue is no conclusive.

+++ b/core/lib/Drupal/Core/TypedData/Type/Binary.phpundefined
@@ -0,0 +1,81 @@
+    elseif (is_resource($value)) {
+      $this->handle = $value;
+    }
+    elseif (is_string($value)) {

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.

+++ b/core/lib/Drupal/Core/TypedData/Type/Date.phpundefined
@@ -0,0 +1,68 @@
+    if ($value instanceof DateTime || !isset($value)) {

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...

+++ b/core/lib/Drupal/Core/TypedData/Type/Date.phpundefined
@@ -0,0 +1,68 @@
+    return (string) $this->getValue()->format(DateTime::ISO8601);

Does this need formatting choice be documented somewhere? Moved into a class constant for a potential derived class?

+++ b/core/lib/Drupal/Core/TypedData/Type/Decimal.phpundefined
@@ -0,0 +1,39 @@
+ * The plain value of a decimal is a regular PHP float. For setting the value
+ * any PHP variable that casts to a float may be passed.

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?

+++ b/core/lib/Drupal/Core/TypedData/Type/Duration.phpundefined
@@ -0,0 +1,64 @@
+    elseif (is_integer($value)) {

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)

+++ b/core/lib/Drupal/Core/TypedData/Type/Duration.phpundefined
@@ -0,0 +1,64 @@
+    return (string) $this->getValue()->format('%rP%yY%mM%dDT%hH%mM%sS');

Suuuuure. How did we arrive to this wonderfully simple string?

fago’s picture

Yay, type magic! Glad I am here. :D

Yay, thanks for the great review!

Couldn't this be somewhere in a class along with drupal_wrap_data as method? Even as a static method? Something :) ?

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..

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.

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.

Any resource or a file resource? Both getValue and getString strongly hints at file resource while setValue is no conclusive.

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?

Does this need formatting choice be documented somewhere? Moved into a class constant for a potential derived class?

Good question. Probably we should add the string representations used to our documentation table.

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?

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:

For example, in this specification, float is a well-defined mathematical concept that cannot be defined in terms of other datatypes, while a integer is a special case of the more general datatype decimal.

Also see http://www.w3.org/TR/xmlschema-2/#decimal.

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)

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()?

Suuuuure. How did we arrive to this wonderfully simple string?

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.

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.

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 :-)

fago’s picture

@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..

fago’s picture

FileSize
32.3 KB

Here is an updated patch based upon the entity-property branch in the sandbox.

fago’s picture

FileSize
44.28 KB

ok, this one should be complete. It contains some bogus references on the property item's of the property API though.

Lars Toomre’s picture

These are some comments from a first read of this patch focused on documentation and high level review.

+++ b/core/includes/common.incundefined
@@ -7067,6 +7067,84 @@ function drupal_get_updaters() {
+ *   - class: If set and 'list' is FALSE, the class to use for creating the
+ *     typed data wrapper. Else the default class as specified for the data type

Would change s/wrapper. Else/wrapper; otherwise,/

+++ b/core/includes/common.incundefined
@@ -7067,6 +7067,84 @@ function drupal_get_updaters() {
+ *   - list class: If set and 'list' is TRUE, the class to use for creating the
+ *     typed data wrapper. Else the default list class as specified for the data

Same as previous comment.

+++ b/core/includes/common.incundefined
@@ -7067,6 +7067,84 @@ function drupal_get_updaters() {
+ *   - settings: An array of settings, as required by the used class. See the

Think it would be clearer to replace 'used class; with something like 'class for type of data'. Used is a confusing adjective here.

+++ b/core/includes/common.incundefined
@@ -7067,6 +7067,84 @@ function drupal_get_updaters() {
+ * @see drupal_get_data_type_info()
+ * @see Drupal\Core\TypedData\Type\Integer
+ * @see Drupal\Core\TypedData\Type\Decimal
+ * @see Drupal\Core\TypedData\Type\String
+ * @see Drupal\Core\TypedData\Type\Boolean
+ * @see Drupal\Core\TypedData\Type\Duration
+ * @see Drupal\Core\TypedData\Type\Date
+ * @see Drupal\Core\TypedData\Type\Uri
+ * @see Drupal\Core\TypedData\Type\Binary
+ * @see Drupal\Core\Entity\Property\EntityWrapper

I believe @see directives should be in alphabetical order. Repeated in multiple places in patch.

+++ b/core/lib/Drupal/Core/TypedData/AccessibleInterface.phpundefined
@@ -0,0 +1,27 @@
+   * Check data value access.

I believe this needs to be Checks rather than Check. Determines might be a better verb in this case.

+++ b/core/lib/Drupal/Core/TypedData/AccessibleInterface.phpundefined
@@ -0,0 +1,27 @@
+   *   Whether the given user has access.

Suggest 'TRUE if the given user has access; otherwise, FALSE.'

+++ b/core/lib/Drupal/Core/TypedData/ListInterface.phpundefined
@@ -0,0 +1,25 @@
+namespace Drupal\Core\TypedData;
+use ArrayAccess;
+use IteratorAggregate;
+use Countable;

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).

+++ b/core/lib/Drupal/Core/TypedData/MissingContextException.phpundefined
@@ -0,0 +1,14 @@
+namespace Drupal\Core\TypedData;
+use Exception;

Again a blank line should be added between namespace and use.

+++ b/core/lib/Drupal/Core/TypedData/StructureInterface.phpundefined
@@ -0,0 +1,115 @@
+   * @throws \InvalidArgumentException
+   *   If an invalid property name is given.

My understanding is @throw directive should appear after @return directive. Repeated elsewhere in patch.

+++ b/core/lib/Drupal/Core/TypedData/Type/Language.phpundefined
@@ -0,0 +1,93 @@
+ * Defines the 'language' data type, e.g. the computed 'language' property of language items.

Exceeds 80 characters... Needs to be shortened.

+++ b/core/lib/Drupal/Core/TypedData/Type/WrapperBase.phpundefined
@@ -0,0 +1,86 @@
+   * @param array $definition
+   *
+   * @param mixed $value;
+   *

Each of these need a description.

+++ b/core/lib/Drupal/Core/TypedData/WrapperInterface.phpundefined
@@ -0,0 +1,83 @@
+   *   (optional) The data value, or NULL if the it is not set. See

Should be 'if it is not set'.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -1998,6 +1998,26 @@ abstract class WebTestBase extends TestBase {
+   * Wraps a data value into a typed data wrapper and executes some basic
+   * assertions.

Needs a short one line description as well as @param and @return directives.

fago’s picture

ad#2

- 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.

I've opened #1789066: Add an undefined data type.

- 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 see. This shouldn't be necessary any more once we do #1789066: Add an undefined data type and a wrapper for it.

+ toArray() does not define whether it returns a plain array or a nested array

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.

+ get(), and set() methods are way too generic, cause clashes if you want to implement other interfaces. Suggestion getPropertyValue(), setPropertyValue()

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?

+ 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

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.

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.

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.

chx’s picture

So 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.

Jose Reyero’s picture

@fago #9

Yeah, nested arrays is something we do not specifically support. . The map well back to nested data structures though...

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)

Looking at the code it seems to be rarely used anyway.

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...

fago’s picture

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.

As 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:

Root level: Config wrapper object. Retrieves metadata about the config object.
Then to get a property wrapper:
- a) If its a primitive, pass on its definition and create the wrapper.
- b) If its a structure, create a ConfigStructureWrapper object, i.e. of type 'config_structure' or so. Pass on its data value and the subtree of metadata so it knows about any contained properties (potentially nested).
Level X: ConfigStructureWrapper. Gets data and a subtree of metadata passed from the wrapper object.
To get a property wrapper it can follow the process as above.

I'm happy to guide you through this on IRC if you like.

- get() and set() methods are poorly named and inconsistent (while 'get' returns a WrapperInterface, 'set' returns a plain value.)

Get and set both work with wrappers. set does not returny anything?

about this last one, never thought of dinamically adding properties to an object?

You can do that, just have getPropertyDefinitions() dynamically return the definitions.

-- 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: Add a uniform Entity Property API

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?

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... )?

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.

fago’s picture

Status: Needs work » Needs review
FileSize
48.92 KB

ad #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..

Status: Needs review » Needs work

The last submitted patch, typed_data.patch, failed testing.

chx’s picture

+++ b/core/lib/Drupal/Core/TypedData/Primitive.phpundefined
@@ -0,0 +1,54 @@
+   * The DECIMAL primitive data type.

I 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:

decimal represents a subset of the real numbers, which can be represented by decimal numerals. The ·value space· of decimal is the set of numbers that can be obtained by multiplying an integer by a non-positive power of ten, i.e., expressible as i × 10^-n where i and n are integers and n >= 0.

vs float

The basic ·value space· of float consists of the values m × 2^e, where m is an integer whose absolute value is less than 2^24, and e is an integer between -149 and 104,

. See below for more.

+++ b/core/lib/Drupal/Core/TypedData/Type/Binary.phpundefined
@@ -0,0 +1,84 @@
+   * The resource URI.

I know setValue has this commented but I think that comment is needed here as well.

+++ b/core/lib/Drupal/Core/TypedData/Type/Boolean.phpundefined
@@ -0,0 +1,40 @@
+    $this->value = isset($value) ? (bool) $value : $value;

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 :)

+++ b/core/lib/Drupal/Core/TypedData/Type/Date.phpundefined
@@ -0,0 +1,69 @@
+    return $this->value;

Isn't this unnecessary , isn't this coming from the TypedData class?

+++ b/core/lib/Drupal/Core/TypedData/Type/Decimal.phpundefined
@@ -0,0 +1,40 @@
+ * The plain value of a decimal is a regular PHP float. For setting the value

Well, see above: decimals can't be represented by floats.

A literal in the ·lexical space· representing a decimal number d maps to the normalized value in the ·value space· of float that is closest to d in the sense defined by [Clinger, WD (1990)]; if d is exactly halfway between two such values then the even value is chosen.
+++ b/core/lib/Drupal/Core/TypedData/Type/Decimal.phpundefined
@@ -0,0 +1,40 @@
+    $this->value = isset($value) ? (float) $value : $value;

Again , what's our feeling about NULL? Always possible to get it?

+++ b/core/lib/Drupal/Core/TypedData/Type/Duration.phpundefined
@@ -0,0 +1,67 @@
+    elseif (is_numeric($value) && (string) (int) $value === (string) $value) {

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."

+++ b/core/lib/Drupal/Core/TypedData/Type/Language.phpundefined
@@ -0,0 +1,92 @@
+   * The data wrapper holding the langcode value.

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?

+++ b/core/modules/system/system.moduleundefined
@@ -1978,6 +1979,105 @@ function system_stream_wrappers() {
+    'entityreference_item' => array(

????? we do not have ER in core.

fago’s picture

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 :)

Yeah, 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 ;)

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."

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?

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?

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.

????? we do not have ER in core.

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.

I 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:

decimal represents a subset of the real numbers, which can be represented by decimal numerals. The ·value space· of decimal is the set of numbers that can be obtained by multiplying an integer by a non-positive power of ten, i.e., expressible as i × 10^-n where i and n are integers and n >= 0.

vs float

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.

chx’s picture

> 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.

fago’s picture

Status: Needs work » Needs review
FileSize
51.46 KB

ok, implemented all three points.

fago’s picture

oh this patch misses service registration so tests will fail. Anyway, it should still help reviewing the typed data API.

Status: Needs review » Needs work

The last submitted patch, typed_data.patch, failed testing.

chx’s picture

Title: Add a TypedData API for allowing metadata to be associated with data » Fix TypedData API
Category: feature » bug
Priority: Normal » Major

I think this is at least a major bug now that the generic set-get brought TypedData in without these fixes in here.

Jose Reyero’s picture

I'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

fago’s picture

Title: Fix TypedData API » Add a TypedData API for allowing metadata to be associated with data
Status: Needs work » Fixed

I think this is at least a major bug now that the generic set-get brought TypedData in without these fixes in here.

All 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.

I'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...?

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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.