Vastly extend the typed data topic

Seems like a great improvement.

1. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * PHP has a couple data types (even if it's not a strictly typed language)
   

   couple "of" data types

2. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * defining classes but if you use classes then you depend on implemntation
   

   implementation

3. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * @endcode
   + * $field_item_list = $entity->get('fieldName');
   + * $field_item      = $field_item_list->get(0);
   + * $property        = $field_item->get('propertyname');
   + * $value           = $property->getValue();
   + * @endcode
   

   This is a great example :)

   Should it be @endcode at the start and end?

thanks chx for sitting down with me, going through all the details and putting this down. It really helps to have an outside view for coming up with docs like this!

Here a few remarks:

1. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   - * The Typed Data API was created to provide developers with a consistent
   - * interface for interacting with data, as well as an API for metadata
   - * (information about the data, such as the data type, whether it is
   - * translatable, and who can access it). The Typed Data API is used in several
   - * Drupal sub-systems, such as the Entity Field API and Configuration API.
   

   I'd keep that as short overview what it is, before going into the motivation.

2. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * defining classes but if you use classes then you depend on implemntation
   

   implementation

3. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * There are three kinds of typed daa: primitive, complex and list.
   

   data. Would be good to list the interface here also.

4. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * \Drupal\Core\TypedData\ComplexDataInterface is a piece of data which
   + * contains named properties where each property is again some sort of
   + * \Drupal\Core\TypedData\TypedDataInterface, can be all three kinds: primitive,
   

   first sentence has no end?

5. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * so only typed data objects can be set or get. Note the base TypedData did
   + * not have methods called get() and set() only getValue and setValue.
   

   I'd say primitive data has no get() and set()

Also, language could be improved a bit in general, but I guess it makes sense to gather more feedback before. Would be good to fix the incomplete sentences for that though -> setting nw.

Some typo fixed.

-* defining classes but if you use classes then you depend on implemntation
+ * defining classes but if you use classes then you depend on implementation

- * There are three kinds of typed daa: primitive, complex and list.
+ * There are three kinds of typed data: primitive, complex and list.

I'm so happy to see this in writing @larowlan and I spent a lot of time figuring this out for DER.

1. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * like int, string, float, array etc. You can also define more types by
   

   These are more then couple.

2. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * So $value is just a raw PHP value. $property is a TypedData object,
   + * $property->getParent() is $field_item, $field_item->getParent() is
   + * $field_item_list, $field_item_list->getParent() is $typed_entity (the
   + * $entity wrapped in TypedData) and $typed_entity->getParent() is NULL.
   

   OMG I spent 3 months learning that.

3. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * For all of these ->getRoot() returns $typed_entity. Some examples where
   

   I didn't know this at all.

4. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * $property->setValue() then $property->onChange() will fire which in turn
   

   This kept me spinning for a while for DER.

1. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * (UrlInterface is a bit of an outlier and the system would work fine without it)
   

   @todo with link?!

2. +++ b/core/core.api.php
   @@ -887,7 +968,6 @@
   - */
   

   typo

I reviewed part of this patch before I ran out of time/energy for the day... I think it's mostly an improvement but it needs a little rewriting. I will try to find some time to do some copy editing tomorrow (someone, please leave me an IRC tell? :) ). Meanwhile, here's a review of what I had time to look at today:

1. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
     * @defgroup typed_data Typed Data API
     * @{
   - * API for describing data based on a set of available data types.
     *
   - * The Typed Data API was created to provide developers with a consistent
   - * interface for interacting with data, as well as an API for metadata
   - * (information about the data, such as the data type, whether it is
   - * translatable, and who can access it). The Typed Data API is used in several
   - * Drupal sub-systems, such as the Entity Field API and Configuration API.
   +/**
   + * @defgroup TypedData Typed data plugins
   + * @{
   

   Um... Can we not change the @defgroup ID? Also this is garbled because the new @defgroup line seems to be within the old one, and the file is going to look like:

   /**
    * @defgroup typed_data (title)
    * @{
    *
   /**
    * @defgroup TypedData Typed data plugins 
   

   This is wrong...

   I'm not sure why the topic name should change from "Typed Data API" ot "Typed data plugins" either.

2. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * like int, string, float, array etc. You can also define more types by
   + * defining classes but if you use classes then you depend on implementation
   + * details so you would need an interface but an interface can not contain
   + * properties. Also, there's no facility for associating metadata with a type.
   + *
   

   This sentence is really really long with no punctuation and a lot of "so" and "but" clauses. Separate into multiple sentences?

3. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * In Drupal 8, a type defined by the TypedData subsystem is a plugin, with
   

   Don't put "Drupal 8" in docs. We'll almost certainly be using them for later versions, and it would be annoying to have to go back and find everywhere it says "Drupal 8" and change it to 8.1 or 9 or 10 or whatever.

4. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * the manager being \Drupal::typedDataManager(). The plugin encapsulates a
   

   Drupal::typedDataManager() is a method. I wouldn't say that is the manager. I would cite the service... something like:

   ... with the manager provided by the foo.bar service.

5. +++ b/core/core.api.php
   @@ -832,40 +832,121 @@
   + * single piece of data, provides access to the metadata, provides validation
   + * capability. Also, there's a tree handling capability see @ref sec_tree.
   

   Needs "and" before "provides validation".

OK, I'm making a patch to copy edit...

So....

I did some copy editing, and also some culling. Some of the stuff in the previous patch, I think belongs in the drupal.org pages under
https://www.drupal.org/node/1794140 (typed data API)
instead of in the api.drupal.org topic (which should just provide the bare minimum).

Interdiff looks fine except for two nitpicks:

1. +++ b/core/core.api.php
   @@ -850,11 +850,12 @@
   + * class is contained in the plugin definition. A default can be set in the
   + * $definition_class property of the annotation class, this can be overridden
   + * in the definition_class property of the plugin annotation itself. Finally,
   + * the plugin deriver can set it too. The metadata object provides information
   

   nitpick: "class, this" should be "class; this" (comma splice otherwise).

2. +++ b/core/core.api.php
   @@ -900,7 +901,10 @@
   + * must have the same base plugin id, however some types (for example field
   + * items and entities) are provided by plugin derivatives and the sub ids can
   

   nitpick: id => ID and ids => IDs (unless you are a psychologist). ;)

The documentation in #14 looks (and reads) great.

Interdiff looks good.

I went back and looked at the patch. Most likely my fault, but I noticed two problems, at the top and bottom of the topic:

a) The first line after

 * @defgroup typed_data Typed Data API
  * @{

needs to be a one-line description of the topic, followed by a blank line. Previous to this patch, we had:

- * API for describing data based on a set of available data types.
- *

We should probably put that back in.

b) At the end somehow we did:

  * @}
- */

Need to put that */ back in, oops!

Other than that I think it's good to go. We should get typed data managers to review though? That would be ... no idea who, it is not in MAINTAINERS that I can see.

Thanks @chx and @fago, these docs look great.

Only one minor observation

+++ b/core/core.api.php
@@ -832,44 +832,113 @@
+ * managed by the typed_data_manager service. Each data object encapsulates a

Is it worth putting the class for the manager service here so api.drupal.org links to it - or does api.drupal.org link using service IDs automagically?

Other than that +1

I am fixing for comments in #16. I am not very sure about using the class name as suggested in #18. I am thinking it makes more sense to keep the service name in documentation. But I don't have any strong opinions.

We don't want people to be using the service class directly. They should use the service name. So I think the docs are good.

Thanks for fixing up the patch. Looks like @fago reviewed it above for a fact check. Let's get it in!

Great improvement, thanks!

I am thinking it makes more sense to keep the service name in documentation. But I don't have any strong opinions.

Ideally, api.drupal.org would be making the connection and make it possible to click on service ids. But if that's not feasible in the rather shot term, maybe just having the service id plus (Default class: ...) for the link would be useful?

I check the patch once again and made a few smaller fixes, and improved the paragraph about definition classes a bit. Changes attached, please review.

Other than that I think it's good to go. We should get typed data managers to review though? That would be ... no idea who, it is not in MAINTAINERS that I can see.

@fago is the typed data maintainer.

Agreed, opened #2551929: Add Typed data to the maintainers txt.

Looking good! I took a careful look through the latest patch, and I have a few suggestions (mostly nitpicks). I think @fago is right about mentioning the class/interface for the manager, and I suggested below where I think it should be done too.

1. +++ b/core/core.api.php
   @@ -834,42 +834,115 @@
   + * managed by the typed_data_manager service. Each data object encapsulates a
   

   Could mention the class/interface of the data manager service here? Preferably interface.

2. +++ b/core/core.api.php
   @@ -834,42 +834,115 @@
   + * type, whether it is translatable, the names of its properties
   + * (for complex types), and who can access it.
   

   nitpick: the line wrapping here needs a bit of attention.

3. +++ b/core/core.api.php
   @@ -834,42 +834,115 @@
   + * Primitive data types are the building blocks for complex and list typed
   + * data. Each primitive data type has an interface that extends
   

   This first sentence sort of implies that the primitive types cannot be used alone... I think/suppose that they can be?

4. +++ b/core/core.api.php
   @@ -834,42 +834,115 @@
   + * List data types, with interface
   + * \Drupal\Core\TypedData\ListInterface, represent data that is an ordered list
   + * of typed data, all of the same type. More precisely, the plugins in the list
   

   Can these lines be rewrapped? Maybe not... not sure if ListInterface fits on the previous line?

5. +++ b/core/core.api.php
   @@ -834,42 +834,115 @@
   + * must have the same base plugin ID, however some types (for example field
   

   Punctuation should be:

   ... base plugin ID; however, some types...

6. +++ b/core/core.api.php
   @@ -834,42 +834,115 @@
   + * an entity field item, the Entity Field API allows you to call
   

   end this line in :

7. +++ b/core/core.api.php
   @@ -834,42 +834,115 @@
   + * $field_item_list = $entity->get('fieldName');
   + * $field_item      = $field_item_list->get(0);
   + * $property        = $field_item->get('propertyName');
   + * $value           = $property->getValue();
   

   We don't normally line up Drupal code like this...

8. +++ b/core/core.api.php
   @@ -886,6 +959,15 @@
   + * - If you need to create a typed data object in code, first get the
   + *   typed_data_manager service from the container or by calling
   + *   \Drupal::typedDataManager(). Then pass the plugin ID to
   

   We could definitely mention the class/interface of the manager here too. Wouldn't hurt.

Looks great, thanks!

Really great work on this. This is one of the gnarliest topics to come to grips with for people new to Drupal 8 (heck, even people old to Drupal 8 ;)).

Committed and pushed to 8.0.x. Thanks!