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.
Problem/Motivation
Float and String will be reserved words in PHP 7. We could easily allow support for PHP 7 by simply renaming Drupal\Core\TypedData\Plugin\DataType\Float and Drupal\Core\TypedData\Plugin\DataType\String to something else.
Proposed resolution
Rename all primitive data types to include 'Data' as a suffix for consistency:
- Drupal\Core\TypedData\Plugin\DataType\Binary to BinaryData
- Drupal\Core\TypedData\Plugin\DataType\Boolean to BooleanData
- Drupal\Core\TypedData\Plugin\DataType\Float to FloatData
- Drupal\Core\TypedData\Plugin\DataType\Integer to IntegerData
- Drupal\Core\TypedData\Plugin\DataType\String to StringData
Remaining tasks
Decide on name.
Write patch.
User interface changes
None
API changes
Several data classes are renamed, but the interfaces are not. Should be a limited impact unless someone has sub-classed these types.
Beta phase evaluation
Issue category | Task |
---|---|
Issue priority | Critical because it is a child issue of a critical issue. |
Prioritized changes | The main goal of this issue is to help make Drupal 8 compatible PHP 7, which has been identified as a release blocker for 8.0.0 |
Disruption | Some class renames will be required when they are directly used, however these classes have interfaces which should be used in most places. The impact of PHP 7 compatibility is greater than the disruption of having to update a few class names. |
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff.txt | 1.28 KB | rteijeiro |
#42 | rename-typed-data-for-php7-2454441-42.patch | 12.76 KB | rteijeiro |
#39 | interdiff.txt | 1.42 KB | rteijeiro |
#39 | rename-typed-data-for-php7-2454441-39.patch | 12.76 KB | rteijeiro |
Comments
Comment #1
benjy CreditAttribution: benjy at CodeDrop commentedComment #2
jibranHow about FloatData?
Comment #3
andypostMaybe
NumberFloat
- add prefix of all number data and fixFloatInterface
Comment #4
benjy CreditAttribution: benjy at CodeDrop commentedComment #5
dawehnerAdding a novice tag.
I am fine with ...Data, it explains clearly what is going on.
Comment #6
mpdonadioShould the interfaces be renamed, too
Float -> FloatData
FloatInterface -> FloatDataInterface
?
Comment #7
benjy CreditAttribution: benjy at CodeDrop commentedThat makes sense to me, although it's another change, I can't see many contrib/custom modules using that interface at this point.
Comment #8
BerdirNot sure about StringData, wondering if StringDataType might better.
Also not sure about the interfaces.
Let's see if we can summon @fago here.
Comment #9
catchBumping priority - this needs to happen before 8.0.0.
Comment #10
fagoI 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.
Comment #11
fagoComment #12
17thColossus CreditAttribution: 17thColossus commentedI'm at the the European Drupal Days in Milan and I'm assigning this issue to me for the Code Sprint.
Comment #13
17thColossus CreditAttribution: 17thColossus commentedComment #14
17thColossus CreditAttribution: 17thColossus commentedMy first patch!
Comment #15
17thColossus CreditAttribution: 17thColossus commentedComment #16
mpdonadioRemoved.
Comment #17
17thColossus CreditAttribution: 17thColossus commentedI managed to read the comment before the removal; thank you for the suggestions, I'll make sure I follow those guidelines next time!
Comment #18
nullkernel CreditAttribution: nullkernel commentedI 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.
Comment #20
nullkernel CreditAttribution: nullkernel commentedChanging the status back because my patch was against beta7 and the patch 17thColossus submitted worked fine.
Comment #21
hussainweb@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.
Comment #22
hussainweb@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.
Comment #23
hussainwebSetting to needs work again. If you believe these are random failures, click "Retest" and just mention why you think these are random.
Comment #24
nullkernel CreditAttribution: nullkernel commentedhussainweb, 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.
Comment #25
hussainwebFixing the failures.
Comment #26
hussainweb@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?
Comment #27
mpdonadio@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.
Comment #28
nullkernel CreditAttribution: nullkernel commented@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.
Comment #29
hussainwebAfter 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.
Comment #30
hussainwebIt 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
toBinaryData
for consistency.Comment #31
hussainwebOops...
Comment #33
hussainwebSomething 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.
Comment #35
rteijeiro CreditAttribution: rteijeiro commentedFixed one last nitpick.
Comment #36
mpdonadioUpdated 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.
Comment #37
mpdonadioCR draft written.
Comment #38
andypostJust 5c, not greped the code
please reorder them alphabetically, people will read that so makes sense
Comment #39
rteijeiro CreditAttribution: rteijeiro commentedThanks for the review @andypost. Reordered!
Comment #40
andypostLooks good to me
Comment #41
klausiShould be IntegerData not IntegerType, correct? Same for the others.
Comment #42
rteijeiro CreditAttribution: rteijeiro commentedNice catch @klausi! Thanks!!
Comment #43
fagoThanks, 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.
Comment #44
alexpottThis 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!
Comment #46
yched CreditAttribution: yched commentedAw, nice !