I might be completely wrong here, but StringLongItem extends StringItem, but removes the max_length restrictions? Shouldn't that be the other way araound: the least restrictive as base?

I guess the even "bestest" option would be an abstract StringItemBase as base, and extend that into StringItem and StringLongItem?

CommentFileSizeAuthor
#4 drupal-stringitembase-2383277-4.patch4.86 KBmrharolda
#1 drupal-stringitembase-2383277-1.patch4.76 KBmrharolda

Comments

mrharolda’s picture

mrharolda commented
Status: Active » Needs review
StatusFileSize
new drupal-stringitembase-2383277-1.patch4.76 KB
mrharolda’s picture

mrharolda commented
Related issues: +#2380391: Fix storage settings StringLongItem

Status: Needs review » Needs work

The last submitted patch, 1: drupal-stringitembase-2383277-1.patch, failed testing.

mrharolda’s picture

mrharolda commented
StatusFileSize
new drupal-stringitembase-2383277-4.patch4.86 KB

Ok, this patch should apply cleanly.

mrharolda’s picture

mrharolda commented
Status: Needs work » Needs review

... and will need a review. You'd think that I'd know how to submit patches by now.

yched’s picture

yched commented
Issue tags: +Entity Field API

I think that's a very good idea. That's how Text*Item are structured for formatted text fields, and it makes more sense IMO.

In #2381079: Adjust storage_settings schema for string_long field type there was the question of how to express the config schema for the settings of those field types, and we removed the "inheritance" on the config schema side. This would make the actual code consistent with that.

Needs review for the testbot to run, but RTBC if tests are green.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

@yched you can always set to RTBC -- the bot will set to CNW if the patch fail.

yched’s picture

yched commented

@chx: sure, i's just that the #5 & #6 were crossposts - the issue was still NW when I started to write #6, so I didn't want to go straight from NW to RTBC :-)

But sure, RTBC +1 (that is, if the bot decides to catch it at last ?)

mrharolda’s picture

mrharolda commented

Too bad there's no 'RBTC + tests ok' status ... or 'RBTC + test pending'?

Anyway: it passed ;)

chx’s picture

chx commented

What I meant to say is that i believe bot is testing RTBC issues as well these days.

alexpott’s picture

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

Nice clean-up. This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 1990524 and pushed to 8.0.x. Thanks!

  • alexpott committed 1990524 on 8.0.x 
    Issue #2383277 by MrHaroldA: StringLongItem should not extend StringItem

Status: Fixed » Closed (fixed)

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