Rename Typed Data classes to support PHP 7

How about FloatData?

Maybe NumberFloat - add prefix of all number data and fix FloatInterface

Adding a novice tag.

I am fine with ...Data, it explains clearly what is going on.

Should the interfaces be renamed, too

Float -> FloatData
FloatInterface -> FloatDataInterface

?

Not sure about StringData, wondering if StringDataType might better.

Also not sure about the interfaces.

Let's see if we can summon @fago here.

Bumping priority - this needs to happen before 8.0.0.

I don't see why we'd rename the interface? Their names are fine and renaming them is unnecessary API change.

I don't think it should be *DataType as the objects are not types, but the instance of it, i.e. data. So StringData and FloatData would be correct, I'm not very fond of the names though.

What about FloatNumber instead of FloatData? I do not have a better suggestion than StringData though.

I'm at the the European Drupal Days in Milan and I'm assigning this issue to me for the Code Sprint.

My first patch!

Removed.

I managed to read the comment before the removal; thank you for the suggestions, I'll make sure I follow those guidelines next time!

I was trying to post a fix during the same time #14 was posted, but Drupal.org was not allowing it.

I took a slightly different approach. I also renamed "Boolean" and "Integer", because I thought it would be consistent. I used "StringType", "FloatType", etc.

I think my clicking the preview button, and deleting and reuploading my patch with a different predicted comment number counted against me. I was banned for hours due to Drupal.org thinking I was posting too much (last time I tried/posted was 8 months ago).

Cheers, it's my first patch (for D8/core) too. Happy code sprinting!

Edit: After looking closely at 17thColossus' patch, it appears mine may fail tests. I ran all the phpunit tests locally and it passed, but I coded the patch against beta7 since I was having trouble getting my existing D8 site to work with 8.0.x checked out.

Changing the status back because my patch was against beta7 and the patch 17thColossus submitted worked fine.

@nullkernel: Great work on your first patch. Just a tip: Try to post an interdiff with your changes. It makes reviewing patches easier and also helps failures like these.

@nullkernel: Just changing the status won't retest the patch. Take a look at the failures and if you believe they were random, click Retest next to the patch to get it to test. I am not sure if the failures here are random.

Setting to needs work again. If you believe these are random failures, click "Retest" and just mention why you think these are random.

hussainweb, the patch I wrote was done independently of the work 17thColossus did. I don't believe an interdiff applies in this case. I changed the status because the testbot automatically updated the status to be negative which does not reflect on 17thColossus' work. In the meanwhile, I am re-setting up my D8 site and database so that it will be based on 8.0.x and not beta7 (Feb 24). Sorry for any confusion.

Fixing the failures.

@nullkernel: Thanks for the clarification. You are right, interdiff does not apply in that case. I somehow didn't get that it was an entirely different effort.

Another note though, just FYI, it is easier for maintainers to commit the last patch in the issue and if you think your patch should not be used, then you should just reupload the intended one. I am assuming that is what you wanted to do by changing the status. However, your patch seemed fine and I agree, we should rename other related data types. There are still other data types in that namespace but they are not close to primitives like the four that are renamed and for that reason, I am not sure if they should be renamed. It just seems inconsistent now. Thoughts?

@17thColossus, I think I was seeing a problem on the machine I was testing on. I also saw the same failure with HEAD. When I test from my primary dev machine, all is OK. That's why I deleted the comment.

@hussainweb, I was thinking the same thing; rename the all of the primitives (ie, extend PrimitiveBase) for consistency but not the others, which would be: Binary, Boolean, Float, Integer, and String. However, I do think that the names should be FooData and not FooType.

@hussainweb

I agree. I got 8.0.x working, and have been running tests locally.. I am having some failures but I think they have nothing to do with this work. I saw that with what I submitted, the testbot only complained about three unit tests, and it was because the unit test itself was using the wrong class name. The testbot ought to pass the patch with your interdiff.

I thought the same thing about renaming stuff like "Email" or "Binary". I figure that it is unlikely that PHP would reserve those as a keyword. They could be renamed to have "Type" (or "Data") after them for consistency, but it would be a bit more work. I think it would be okay to leave them as they are (then again, it would be nice to be consistent). However, other languages reserve primative types such as "Integer" or "Boolean" and conceivably PHP (or Hack) could go down that route.

I went with "Type" over "Data", because I think "Type" describes a class wheras "Data" describes an instance of a class. That said, I could see it being named either way.

After the discussion above, I am leaning towards a Data suffix too. I think it becomes much more readable if you were to write $int = new IntegerData(); rather than $int = new IntegerType();.

I will try to get a patch tonight or tomorrow. Meanwhile, setting this to Needs work in case someone wants to take a shot.

It seems the test runner has stopped for #25.

I am attaching a patch to change all suffixes from Type to Data as discussed in comments since #25. Also, as per #27, I am also renaming Binary to BinaryData for consistency.

Oops...

Something went wrong with the patch in #30. I am not sure what but the patch did not apply correctly except for the file renames and hence, all those errors. There was an extra space before every line and the diff header was different than usual.

Anyway, the interdiff was correct and I reapplied it to create a new patch (attached). The interdiff in #30 can still be used for review.

Fixed one last nitpick.

Updated IS; added Beta Eval.

Since the patch touches core/config/schema/core.data_types.schema.yml, should this be updated to at least include BinaryData?

I'll write a CR.

CR draft written.

Just 5c, not greped the code

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -132,14 +132,14 @@ public function createInstance($data_type, array $configuration = array()) {
-   * @see \Drupal\Core\TypedData\Plugin\DataType\Integer
-   * @see \Drupal\Core\TypedData\Plugin\DataType\Float
-   * @see \Drupal\Core\TypedData\Plugin\DataType\String
-   * @see \Drupal\Core\TypedData\Plugin\DataType\Boolean
+   * @see \Drupal\Core\TypedData\Plugin\DataType\IntegerType
+   * @see \Drupal\Core\TypedData\Plugin\DataType\FloatType
+   * @see \Drupal\Core\TypedData\Plugin\DataType\StringType
+   * @see \Drupal\Core\TypedData\Plugin\DataType\BooleanType
    * @see \Drupal\Core\TypedData\Plugin\DataType\Duration
    * @see \Drupal\Core\TypedData\Plugin\DataType\Date
    * @see \Drupal\Core\TypedData\Plugin\DataType\Uri
-   * @see \Drupal\Core\TypedData\Plugin\DataType\Binary
+   * @see \Drupal\Core\TypedData\Plugin\DataType\BinaryData

please reorder them alphabetically, people will read that so makes sense

Thanks for the review @andypost. Reordered!

Looks good to me

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -132,14 +132,14 @@ public function createInstance($data_type, array $configuration = array()) {
+   * @see \Drupal\Core\TypedData\Plugin\DataType\BooleanType
    * @see \Drupal\Core\TypedData\Plugin\DataType\Date
+   * @see \Drupal\Core\TypedData\Plugin\DataType\Duration
+   * @see \Drupal\Core\TypedData\Plugin\DataType\FloatType
+   * @see \Drupal\Core\TypedData\Plugin\DataType\IntegerType
+   * @see \Drupal\Core\TypedData\Plugin\DataType\StringType

Should be IntegerData not IntegerType, correct? Same for the others.

Nice catch @klausi! Thanks!!

Thanks, this looks good. +1 on renaming all of the primitives for consistency, that makes sense. Patch looks good now also, thus RTBC given this comes back green.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 58481b7 and pushed to 8.0.x. Thanks!

Aw, nice !