Updated: Comment #3

Background

In our schema definitions we can either declare text as type 'varchar' if it's not very long (~ single line textfield) or as type 'text' if it can be long (~ multi-line textarea). Many textareas have a text format attached to them and this is stored in the schema along with the actual text in a separate format column.

Problem/Motivation

The current field items in core are as follows (the table shows the respective plugin IDs):

Schema type Without format column With format column
varchar string text
text
  • text_long
  • text_with_summary

As can be easily seen this is incomplete.

A field type with schema type 'text' without a format column is needed.

Proposed resolution

Provide a field type with schema type 'text' without a format.

To be as consistent as possible with the current naming scheme (which is not very consistent; see above), a plugin ID of string_long is used.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#45 2181549-45.patch1.93 KBandypost
#30 2181549-29-string-long-item.patch1.93 KBtstoeckler
#29 2181549-24-29-interdiff.txt1015 byteststoeckler
#24 2181549-24-string-long-item.patch2.26 KBtstoeckler
#24 2181549-20-24-interdiff.txt1.39 KBtstoeckler
#20 2181549-20-string-long.patch2.62 KBtstoeckler
#18 2181549-18-string-long.patch29.96 KBtstoeckler
#16 2181549-13-string-long.patch241.72 KBtstoeckler
#13 1423244-13-text-format-allowed-formats.patch20.49 KBtstoeckler
#13 2181549-3-13-interdiff.txt2.28 KBtstoeckler
#3 2181549-3-text-long-formatted.patch1.69 KBtstoeckler
#1 2181549-1-text-long-formatted.patch8.84 KBtstoeckler

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new 2181549-1-text-long-formatted.patch8.84 KB

Here we go.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: +D8MA, +SprintWeekend2014, +Entity Field API
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2181549-3-text-long-formatted.patch1.69 KB

Discussed this @Berdir in IRC.

I had previously overlooked the 'text' item with also has a format, which made the previous patch introduce a terrible inconsistency. Updated the issue summary for the new approach.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue summary: View changes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue summary: View changes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Title: Rename TextLong item to TextFormatted and provide a *real* text long item » Provide a field item with schema type 'text' without a formatter column
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringLongItem.php
@@ -0,0 +1,63 @@
+class StringItem extends FieldItemBase {

this should be StringLongItem, right?

why doesn't this extend StringItem? Probably because this one does not have a max_length? Please add a comment.

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringLongItem.php
@@ -0,0 +1,63 @@
+

    Clearly a huge issue: This empty line is not needed. 

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringLongItem.php
@@ -0,0 +1,63 @@
+          'default' => '',

    I try to figure out why the TextLongItem does not set a default value.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The naming in the issue summary is a bit confusing. A format*ter* is usually a field formatter, what we're talking about here is a text format.

As @klausi said, the class name is wrong.

Also the "entity" in entity field type is unecessary, we tried to remove that but maybe missed a few.

Are there examples in core that should use this instead of string? We need a way to test this, and being able to use it on an existing entity might be an easy way to do that.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

@everyone: thanks for the reviews!!! very helpful!

Re #7:
- 1. Yes, will fix that, sorry.
- 2. As I learned the hard way in [#https://drupal.org/node/2177799] the fact that field item classes use static:: (i.e. late static binding) for their property definitions means that the static is shared with potential base classes. Since we need different propertyDefinitions for the StringLongItem class this would be a problem. I personally think that we should stop using LSP for propertyDefinitions but it seems that would be a larger issue.

Re #8:
- 1. Will fix
- 2. We are rather inconsistent in core schemas whether or not to provide a default, although in most cases we do. This is also taken over directly from the StringItem, if I'm not mistaken. I do not have a strong opinion on defaults or not myself. I generally think having explicit fails rather than hiding them with a senseless default value makes more sense, but that also would be a larger issue.

Re #9:
- 1: Ooops, sorry. Will remove references to "formatter" from the issue summary. Yes, that is confusing indeed.
- 2: Will remove the "entity" part. I think that was also just copied over, but alas...
- 3: There are a couple use-cases in core, e.g. aggregator feed description. Utilizing that for the property definitions wouldn't provide any "actual" test coverage, but it can't hurt. Will include in the next re-roll.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

About the static, you will be pleased to see that #2002134: Move TypedData metadata introspection from data objects to definition objects will make that much, much nicer :)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2181549-3-13-interdiff.txt2.28 KB
new 1423244-13-text-format-allowed-formats.patch20.49 KB

Re #12: Ahh, that's very interesting. Then I don't need to open an issue to get rid of the LSB :-)

Here's the updated patch. I grepped for 'text_long' and the aggregator feed description is the only one that makes sense to convert. There might be something that I missed, though.

Status: Needs review » Needs work

The last submitted patch, 13: 1423244-13-text-format-allowed-formats.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Title: Provide a field item with schema type 'text' without a formatter column » Provide a StringLong field item with schema type 'text' without a 'format' column
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2181549-13-string-long.patch241.72 KB

Oops, that was the completely wrong patch. Interdiff is correct, though, I think.

Status: Needs review » Needs work

The last submitted patch, 16: 2181549-13-string-long.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2181549-18-string-long.patch29.96 KB

Trivial merge.

Status: Needs review » Needs work

The last submitted patch, 18: 2181549-18-string-long.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2181549-20-string-long.patch2.62 KB

Man, I seriously managed to botch up the patches in this issue. Incredible... Apologies to those following along at home.

This one should be the proper patch. It actually works on my system. No interdiff because those patches were just too badly messed up.

The more I spend time on this, however I think we should seriously revamp our typed data implementation in this field. Since the whole long/not-long distinction is only the schema type (i.e. hidden pretty far away from the developer) couldn't we try to eliminate that entirely and generate a different schema based on field settings only? Either we add a dedicated setting for the schema type, or we could even check the maxlength setting and then dynamically switch to varchar for maxlength <= 255.

Then we could just have one String / StringItem and FormattedText / FormattedTextItem (and FormattedTextWithSummary ...).

Thoughts?!

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#1856562: Convert "Subject" and "Message" into Message base fields, +#2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), +#2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities

RTBC +1

Another use-case for this item is #1856562: Convert "Subject" and "Message" into Message base fields

Also there's discussion about formatters and widgets #2198917: Use the string field type for the node title field

@tstoeckler about schema generation follow #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Assuming #21 wanted to RTBC?!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 2181549-20-string-long.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2181549-20-24-interdiff.txt1.39 KB
new 2181549-24-string-long-item.patch2.26 KB

Updated for the new field item style.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

We *could* now extend StringItem if we want to. Would allow us to re-use the propertyDefinitions() method....

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

RIght I thought about that, as well. Then we'd have to override the schema but more importantly we'd inherit the 'max_length' setting including the respective validation constraint. At least in the schema we cannot validate that, as 'blob' fields do not have a length. And I'm thinking if you want a max_length you'd better use 'string', no? I mean theoretically you could define a max_length of e.g. 3000 which you cannot save into a 'varchar' column and which we could still validate in PHP, but is that really a sensible use-case?

tl:dr; I'm not really sure about inheriting String, what do other's think?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

At the same time you can set max-length to 0 to skip validation, but I see no reason in constraints in this simple type

yched’s picture

yched commented

FWIW, text_long field type has a max_length setting. It's not set as a FAPI constraint on the textarea widget, but is part of the field-level constraints, which is more important.
So, for consistency, not sure why we'd not have it on string_long. "no longer than 500 chars" is a valid use case.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2181549-24-29-interdiff.txt1015 bytes

Consistency is a pretty strong argument. Let's do it then.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new 2181549-29-string-long-item.patch1.93 KB

d.o weirdness...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Also note that all of the database engines primarily supported by core are supporting VARCHAR(0...65535):

  1. MySQL supports VARCHAR(0...65535).

    Specifically for MySQL, usage of VARCHAR instead of TEXT means that a column can be part of an index.

  2. PostgreSQL treats all CHAR, VARCHAR, and TEXT internally as TEXT, and thus the length constraint of VARCHAR is unlimited.

    (It is even claimed that avoiding CHAR/VARCHAR makes Postgres faster.)

  3. SQLite only supports TEXT. There is no maximum length and not even a constraint/truncation.

This is only meant to clarify the situation and not meant to hold up this patch.

However, given that string_long is turned into an explicit new data type here, and in light of the support for VARCHAR(>255) in the primary db engines, we might want to consider to adjust the field schema definition and/or Schema API in a follow-up issue, so as to allow a DB engine to use VARCHAR instead of TEXT where applicable/sensible.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Now there's string field with formatter and widget - #2198917: Use the string field type for the node title field
Suppose the type here could re-use "string" formatter at least, also probably the text_area widget could be inherited from "string_long" so contact module will not depend on text module anymore #1856562: Convert "Subject" and "Message" into Message base fields

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Suppose we need this asap, then add the widget and formatter in #1856562: Convert "Subject" and "Message" into Message base fields

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Also wondering how this relates to #2149845: Convert the description field of the 'aggregator_item' entity to a text field.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2149845: Convert the description field of the 'aggregator_item' entity to a text field

Suppose Item description field should be converted as well to proper storage (text-big)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2181549-29-string-long-item.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

30: 2181549-29-string-long-item.patch queued for re-testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

back to rtbc, some strange uasort() failure

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2181549-29-string-long-item.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

30: 2181549-29-string-long-item.patch queued for re-testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC after random bot failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2181549-29-string-long-item.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

30: 2181549-29-string-long-item.patch queued for re-testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 2181549-45.patch1.93 KB

re-roll after #2145103: Provide non-configurable field types for entity created, changed and timestamp fields

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 8833cc0 and pushed to 8.x. Thanks!

  • Commit 8833cc0 on 8.x by alexpott: 
    Issue #2181549 by tstoeckler, andypost: Provide a StringLong field item...

Status: Fixed » Closed (fixed)

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