Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

Issue summary: View changes

clarify what previous issue was about

Everett Zufelt’s picture

Title: Increase size of "alt" and "title" text for images. » Increase length of "alt" and "title" text for images.
Issue tags: +Accessibility
BTMash’s picture

From CCK image fields, the max length for the alt text was 80 and the max length for titles was 500. Anyone that comes over doing an upgrade from D6 to D7 doing an upgrade with titles that were longer than the current maximum (128) would (and have in my case since I have titles that are around 300 characters in length) run into issues. So it *should* match the functionality that it is replacing.

On a side note, a contrib solution to resolve this is currently *not* possible. Field Schemas cannot be altered (see #691932: Add hook_field_schema_alter()) to allow for more flexibility and until that is possible, this has to be a core issue.

Everett Zufelt’s picture

Priority: Normal » Major

Bumping to Major as, AFAIK, this is the recommended upgrade path from CCK Image field, and there will be data loss in situations where @title is longer than 128 chars.

jenlampton’s picture

Status: Active » Needs review
FileSize
2.23 KB
1.89 KB

This patch depends on #1015916: Image field "title" allows more data than database column size.

Increased alt text to 512.
Increased title text to 1024.

Status: Needs review » Needs work

The last submitted patch, increase_length_alt_title-1353030-4.patch, failed testing.

theborg’s picture

Status: Needs work » Needs review

D7 patch is working ok, with some minor modifications:
'#maxlength' => 128, // See http://www.gawds.org/show.php?contentid=28.' <= '28 .' and removing the variable deletion part as is included in #1015916.

jenlampton’s picture

I think the patch is failing testing because it needs the other patch first :/ maybe we should wait.

In the mean time, here'e the updated D7 version.

theborg’s picture

Status: Needs review » Needs work

Image widget fails editing already created content (size of alt and title image fields of 128 char.)

BTMash’s picture

This would need an update hook to change the size of the alt/title fields for existing images to be 512/1024 characters. You can take a look at post #68 in #1015916: Image field "title" allows more data than database column size. or how the update on the field dimensions changes up the schema.

jenlampton’s picture

There shouldn't be any already-existing content for D8, but yes. We need a new update hook for D7. Good catch :)

dotman’s picture

is there a working patch for D7 yet for new and existing content?

thanks.

jenlampton’s picture

@dotman this needs to be fixed in D8 before it can be fixed in D7. If you want to help us, you could provide a working D7 patch for others to use in the mean time.

theborg’s picture

@dotman, here is a D7 patch merged with db update code http://drupal.org/node/1015916#comment-5033974 that BTMash pointed out, for the moment it includes http://drupal.org/node/1015916#comment-5317356 and the tests. I'll post another interdiffed patch.

theborg’s picture

D7 patch with db update code (credits to BTMash)

mgifford’s picture

Status: Needs work » Needs review

Go bot go!

BTMash’s picture

FileSize
1.81 KB

The last patch had failed for some reason. I'm rerolling the patch in #4.

Status: Needs review » Needs work

The last submitted patch, 1353030.patch, failed testing.

BTMash’s picture

FileSize
2.89 KB

Forgot to alter the tests. I wonder if it would be better to not use hardcoded values for the sizes if #691932: Add hook_field_schema_alter() gets in.

mgifford’s picture

Status: Needs work » Needs review

Bots?

BTMash’s picture

FileSize
3.58 KB

Ok, new version of patch which checks against the length of the schema.

jenlampton’s picture

Status: Reviewed & tested by the community » Needs work

I just tried to insert longer alt and title text, and I'm back to getting the PDO error:

PDOException: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'field_image_alt' at row 1:

edit: Well, duh, of course I am. Let me re-install, and try again...

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

Okay, on a fresh drupal 8 install, this works exactly as intended. :)

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

The patch looks great to me. I think we should probably add test coverage for it.

BTMash’s picture

Issue tags: -Needs tests

Test coverage had already been added from #1015916: Image field "title" allows more data than database column size. so the values were modified so the tests reference the values of the schema rather than hardcoded values.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Ah, alright, that works for me.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Patch (to be ported)

Works for me. Committed/pushed to 8.x.

BTMash’s picture

Status: Patch (to be ported) » Needs review

Marking as needs review on patch #14

xjm’s picture

Status: Needs review » Needs work

#14 doesn't seem to match the latest patch in #20? Is there a reason for this I'm not seeing?

If not, there's a few other things to clean up in there. :)

theborg’s picture

@xjm, I guess it's because it was a D7 patch, so go ahead and change it as you see fit. Thanks.

BTMash’s picture

Assigned: Unassigned » BTMash

Going to assign to myself so I remember to make the appropriate changes to the patch.

BTMash’s picture

Assigned: BTMash » Unassigned
Status: Needs work » Needs review
FileSize
4.61 KB

New patch. Backported #20 and combined it with a schema update to increase the size of the current set of image fields. #14 was revised so that it utilized _field_sql_storage_tablename and _field_sql_storage_revision_tablename

BTMash’s picture

FileSize
4.56 KB

Per @xjm's suggestion from irc, change function summary to one line summary.

BTMash’s picture

FileSize
4.56 KB

Make the summary plural.

xjm’s picture

Issue tags: +Novice, +Needs manual testing

Or, well, third person. But yeah, that's nitpicked enough for me. :)

The patch looks good to me, including the added update function. Let's get some manual testing on the patch to ensure the update function works properly:

  1. Install Drupal 7.x-dev or latest 7.x HEAD.
  2. Create some image data with alt and title attributes.
  3. Apply the patch.
  4. Run update.php
  5. Verify that the database tables are upgraded properly, the image field data is unaffected, and that nothing else gets broken.

The above would be a good novice task.

philosurfer’s picture

on 7.10

$ patch -p1 < 1353030_4.patch
patching file modules/image/image.field.inc
Hunk #1 FAILED at 404.
Hunk #2 FAILED at 413.
2 out of 2 hunks FAILED -- saving rejects to file modules/image/image.field.inc.rej
patching file modules/image/image.install
Hunk #2 succeeded at 386 (offset -9 lines).
patching file modules/image/image.test
Hunk #2 FAILED at 792.
1 out of 2 hunks FAILED -- saving rejects to file modules/image/image.test.rej

xjm’s picture

Thanks philosurfer--can you try and see if it works properly with 7.x-dev?

philosurfer’s picture

Status: Needs review » Fixed

I confirm Patch works in 7.x-dev - Jan 02 2012 :)

philosurfer’s picture

Status: Fixed » Reviewed & tested by the community

status update.

droplet’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

1KB content for TITLE attribute ? It's crazy. You able to get first THREE paragraphs texts of this article into TITLE attribute.

(also larger varchar column increasing DB memory needs if im not wrong.)

+++ b/modules/image/image.field.incundefined
@@ -404,7 +404,7 @@ function image_field_widget_process($element, &$form_state, $form) {
     // @see http://www.gawds.org/show.php?contentid=28

The ref link told us: Don't write LONG alt but it allowed 512 now.. It should be remove @ref IMO.

EDIT:
http://www.w3.org/WAI/GL/WCAG20/tests/test60.html

-10 days to next Drupal core point release.

droplet’s picture

and IE9 only display first 5xx chars:
http://jsfiddle.net/Zy598/

jenlampton’s picture

Status: Needs work » Reviewed & tested by the community

I agree, these sizes are quite, large. But this is what got committed to D8, so we might as well be consistent in D7. There's far less harm in saving more data than necessary than there is in saving less than necessary. Let's just get this in and move on to other battles :)

droplet’s picture

Status: Reviewed & tested by the community » Needs work

@jenlampton,
do you mean we should start another same issue to fix sizes in D8 ?? D8 is Dev version, we able to revert any commits.. but once D7 committed..... it will be ONE more upgrade path issue.

I agree, these sizes are quite, large.

It's enough to switch back

There's far less harm in saving more data than necessary than there is in saving less than necessary.

then I would suggest 3000. (the maximum value for most DB varchars).

jenlampton’s picture

Status: Needs work » Reviewed & tested by the community

If you don't like the sizes already agreed on, can you suggest something you think would be more reasonable, and provide a patch? If not, let's make some forward progress.

droplet’s picture

- with #2 upgrade path issue, #40 IE issue, 512 is good enough.
- remove: @see http://www.gawds.org/show.php?contentid=28 || back to 128.

I'm willing to send a patch if you're happy with it.

+++ b/modules/image/image.installundefined
@@ -395,6 +395,41 @@ function image_update_7003() {
+    'type' => 'varchar',
...
+    'not null' => FALSE,
...
+    'type' => 'varchar',
...
+    'not null' => FALSE,

this update changing users custom settings.

-10 days to next Drupal core point release.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Still looking for someone to actually test the upgrade path, please, not just whether the patch applies. (The fact that it passes testbot tells us that it applies.). See #34. (If you did go through these steps, it's helpful to specifically describe what you did and what the results were.) Thanks everyone!

amateescu’s picture

Title: Increase length of "alt" and "title" text for images. » Increase length of "alt" and "title" text for images
Version: 8.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

I manually tested the patch from #33 using the steps from #34 and everything looks good. Data kept intact, field size increased in the db and nothing else got broken :)

jenlampton’s picture

Also tested #33 again manually, database updated appropriately.

webchick’s picture

Priority: Major » Normal

Since this is a feature request, it'll have to wait until we're under thresholds.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

This looks like it wound up being accidentally committed as part of #131737: Ensure that the Return-Path is set when sending mail on both Windows and non-Windows systems., so I'm moving the issue to "fixed" for the time being.

Since the D8 version of the patch went in without an update function, I think that means this actually sort of counts as a D7 bugfix (since there needed to be an update function against D7 or D8 for this at some point; otherwise, existing image fields would never have been converted)?

catch’s picture

Category: feature » bug

For posterity, moving this to a bug report per #2:

From CCK image fields, the max length for the alt text was 80 and the max length for titles was 500. Anyone that comes over doing an upgrade from D6 to D7 doing an upgrade with titles that were longer than the current maximum (128) would (and have in my case since I have titles that are around 300 characters in length) run into issues. So it *should* match the functionality that it is replacing

catch’s picture

Issue tags: +D7 upgrade path

And tagging.

droplet’s picture

Sadly to see this **accidentally** committed. (Should it alter commit message?)
http://drupalcode.org/project/drupal.git/commit/14a6ec5b916f706d154504ba...

It's may bring some new issue:
- Browsers: Drupal supports IE and IE only show first 5xx chars when you hover the links (http://jsfiddle.net/Zy598/)
- UI: textfield can't show 1000 chars
- Performance: maybe use more memory (http://stackoverflow.com/questions/1151667/what-are-the-optimum-varchar-...)
- meaningless ref ? (@see http://www.gawds.org/show.php?contentid=28)

It's crazy, we don't think about the average usage.

Does it CCK upgrade handle in CORE ?

BTMash’s picture

well, now, atleast a form alter could be done to limit down on the size of a field presented to the user if needed. I personally did need the min. 500 character limit on both titles and alts and others had talked about needing more for *months* which is why we have the much larger limits) and given that there is no way to alter the schema on fields (and that cck did have the 500 character limits, this makes sense. I'm fine with 500 char limits on both alt and title (and am happy to rtbc a patch that satisfy atleast those limits and with tests though none of the tests should really change), but I've been at this issue for the past 11 months and would rather see someone else lead the charge codewise on this issue at this stage.

BTMash’s picture

And since this issue was committed in error, then rolling back on the 7.x patch for all the image related portion from #131737: Ensure that the Return-Path is set when sending mail on both Windows and non-Windows systems. and strictly committing the code from that issue would be a good idea. As much as I hate to say it, its only fair.

BTMash’s picture

@droplet, Please create a new patch as you see fit. However, the schema portions that you pointed out change user settings actually do not; they are exactly the same as in the field schema but that data needs to be included in for the particular column change.

droplet’s picture

@BTMash,

I was thought if users' set that columns to 3000 and it's updates will change them back to 1000 or 500. (but seems we never care about this on other updates. ** It won't lost user data **)

The patch only contains one update function. what I think modify it yourself to increase the value and form alter to limit it are same case / work. May I overlooked something. (I don't think everywhere in your sites needed 1000 for title of image field)

what I see it's because missing #691932: Add hook_field_schema_alter() so increasing the value. I don't want being picky but I will start an issue after this hook get commited. It's no fun tho and maybe disallow to backport to 7.x

anyway to notice @catch / @webchick to amend git commit message :)

BTMash’s picture

@droplet,

I'm having a little bit of difficulty understanding what you wrote I apologize in advance if I didn't answer your questions/concerns as you might have liked.

For your question from #52, this patch would get us on the way to handle the cck update for image fields into core. Since the cck image fields had the larger alt/title length values (though they are could technically be much larger since they used a data column which was a text field), they do not handle the base line of image without this patch.

Now for #56. You're right that if users set their column size to some other large value, there is a possibility that data could be lost. Though trying to figure out which tables are larger / whatnot could be quite problematic since reading the schema is not going to give you the accurate value (since #691932: Add hook_field_schema_alter() has not been fixed, we have no way of knowing if the user might have altered the table. We also have no way of dynamically changing the size of fields in the form alter if there is not field schema alter to go with it; the user will just get WSOD). And reading through the history of this issue from #1015916: Image field "title" allows more data than database column size. would give you an idea of why we cannot have variable set by the user to change the size as needed or how the ui was not determined for the fields. As I mentioned...for one of the sites I have, I do need the larger limits on the image fields (and yes, they do get used everywhere re: the images) but I'm also saying that having the lower end of the limits (500 on both alt and title) would be swell. So, this seems like the best compromise. A relatively large field which can be trimmed down in terms of user input via hook_form_alter but won't break the site.

droplet’s picture

It's also the comment for someone wants 3000 chars.

@BTMash,

You're great, you haven't drop me and keeping reply :)

Quickly read #691932 again, my conclusion:
- Not all commenter on #691932 said YES to 500/1000 chars.
- Supporters said "I need it", no meaningful reason (It's bias, im an objector, hehe)

Increase value:
Pros:
- able to write more chars

Cons:
(I cut off my #52 list)
- IE rendering limits
51x Latin chars / tested with 4xx Chinese words, IE takes few sec to display it, **sigh**
- DB Performance (Honestly, I don't know if it's a problem or not)
- Needs contrib module to alter form to limit chars

upgrade path issue

I think it isn't. I tried once to upgrade a site from D6 to D7 when the stable version comes out. AFAIK, we require D7 CCK to upgrade it.

Upgrading to Drupal 7
#D7CX: ImageField is being added to Drupal 7 core! So you don't need a version of it in Drupal 7. To upgrade ImageField to Drupal 7, install the Drupal 7 CCK Package and use the Content Migrate to upgrade ImageField to Drupal 7.

http://drupal.org/project/imagefield

Why can't move its patch into Drupal 7 CCK Package ??

#2
On a side note, a contrib solution to resolve this is currently *not* possible. Field Schemas cannot be altered (see #691932: No hook_field_schema_alter()) to allow for more flexibility and until that is possible, this has to be a core issue.

Patch #33, it used image_update_7004 to update exist fields (extremely simple function). I think you can write a module add an extra #submit function during field creation.

Why do you need it ?
I never seen a site writes 500 / 1000 chars in IMAGES TITLE. Do your guy use TITLE as description?
Only local images are allowed.
Titlte texts Titlte texts Titlte texts Titlte texts
Titlte texts Titlte texts Titlte texts Titlte texts
Titlte texts Titlte texts Titlte texts Titlte texts

Try field collection if you need it.

After Patch:
for someone who don't need 500 or 1000 chars. what can they do ??
1. hacking form to limit it
2. alter DB column (of course it's an option)

for someone wants 3000 chars, what they need to do ??
1. hack form to increase limit
2. alter DB column

** Don't request to increase it anymore **

No one supporting me here, I won't send a patch to testbot. He is sooo busy.

Thanks ALL.

BTMash’s picture

ok, then would you consider 500 chars for alt / 500 chars to title good? I'm frankly ok with that but I can't see it going any lower. As per #3, there would be data loss for anyone migrating from cck imagefield in D6 to core image field in D7. I had gotten around this by going via the migrate module. That is where I had encountered these errors and that in itself should be a dealbreaker for keeping the status quo for max 128 char limits. As far as performance, the size of the indexes increases to whatever your varchar max size is if that is your index but that is about it (in our case, it is not an index so as far as I know, it should not be an issue). I can't use field collection because the site is already developed and again, it goes against the point of having a minimum for the upgrade from D6 to D7. If #691932: Add hook_field_schema_alter() can get in, there is little issue since there is now a safe way to update the size of the fields and kept them at that for any new fields of that type that are created (though arguably, telling everyone that upgrades their d6 site to d7 to also download a contrib module because it fixes an upgrade path bug seems like a bad idea) to then download a contrib module. Not everyone knows how to alter the db column (I do and have done that; but you can't say that is the right way to do things when, if you fetch the schema for the field, it does not match what is actually defining the column). Hacking the form is easy because Drupal provides hooks for it; the field schema doesn't and that is the difference in this scenario.

webchick’s picture

Can someoen summarize for me what's happening here? Does this need to be rolled back or? We're back under thresholds (or were this morning) so although this was accidentally committed, I can't really roll it back now for that reason.

It sounds like this would only adversely affect people who've hacked their core DB schema, which I'm not too concerned about, personally. If you've hacked core, you can likely hack around this update function too.

droplet’s picture

@webchick,
Because it's accidentally committed, missing a correct commit message.

@BTMash,
500 is a better than 1024 and solve IE problem.

created a module for myself to lower its value and others who want 2000 or more.

It's super simple module with 100 lines.
http://drupal.org/sandbox/droplet/1416898

xjm’s picture

Regarding #61:

  1. This patch was committed with another unrelated patch (#131737: Ensure that the Return-Path is set when sending mail on both Windows and non-Windows systems.) in a single commit. So, git blame would return totally confusing info for lines from either patch. Ideally, we should git revert 14a6ec5b and then make two separate commits.
  2. Based on #52, it sounds like there is some concern that the column length should be reduced.
Alan D.’s picture

Follow up on #52 final point and more of a global thing: getting fully anal about the size, it is 2^n - 1 due to the NULL string termination.

But after 255 it probably doesn't matter. MySQL uses a text type internally. PostgeSQL is similar, but I do not know the point where storage switches from inline table data to referenced file, from the manual "Long values are also stored in background tables so they do not interfere with rapid access to the shorter column values.".

So, from this point of view, I would considered 255 to be a defacto limit on varchars, switching to text after that. It would be nice to see some form of standard added about this. This is a fairly nice guideline on what is a 1 liner and what is chunk of text (ie: title/alt attribute c/f caption).

Regarding image_update_7004()
Can we safely do this? From what I gathered we couldn't update the field schema once created, ref: #937442: Field type modules cannot maintain their field schema (field type schema change C(R)UD is needed). And this is not batched, so that is an issue to isn't it?

droplet’s picture

@Alan D.

Thanks for the explanation.

for modules maintain, I think it's right. That's no directly way to alter field schema. But the patch is deeply hacking the CORE.
In #61, I built a module, it's re-update the schema after fields creation.

BTMash’s picture

I'm not sure how I should be responding at this stage. I mentioned being fine with a lower limit (500/501 on both is fine with me; make a patch and I'll rtbc it). However, I read up with what happens in mysql > 5.0.3 and so long as it is less than 65535 bytes (so in our case, about 20k characters), it will get stored in the table (checking the explain, I do not see any temp tables being created). As far as update_7004 and altering the field schema, update_7002 does something similar, text module does the same so it *has* been done in the past. Making a batch of this seems like a good idea though I'd need help with it since I am unfamiliar with the process.

webchick’s picture

Has anyone actually been reporting problems about this upgrade path from the field?

For example, #1425342: Drupal core upgrade from 7.10 to 7.12 causes menu block to fail has been positively blowing up the past day or so, and that's for a upgrade path problem with a contributed module that only about 10% of Drupal sites are running. I would expect that if a core module that almost all sites are running were having a similar problem we would've heard something by now. Or is there chatter happening elsewhere in the forums/issue queue?

catch’s picture

Regarding image_update_7004()
Can we safely do this? From what I gathered we couldn't update the field schema once created, ref: #937442: Field type modules cannot maintain their field schema. And this is not batched, so that is an issue to isn't it?

Because field storage is pluggable, an alternative storage engine would need to implement the same upgrade path (for example another SQL storage engine like per-bundle storage that also has column definitions and limits, MongoDB wouldn't need an update for this since it's schemaless anyway).

This not being batched could be an issue if a site has several image fields and each one has a lot of data saved for it. Since there's no indexes changed, nor any need to move data around in PHP, it may not be a problem in practice with this particular update though.

But after 255 it probably doesn't matter. MySQL uses a text type internally. PostgeSQL is similar, but I do not know the point where storage switches from inline table data to referenced file, from the manual "Long values are also stored in background tables so they do not interfere with rapid access to the shorter column values.".

So, from this point of view, I would considered 255 to be a defacto limit on varchars, switching to text after that. It would be nice to see some form of standard added about this. This is a fairly nice guideline on what is a 1 liner and what is chunk of text (ie: title/alt attribute c/f caption).

@Alan D., the internal text storage didn't come up as an issue on #1284658: MySQL strict mode is causing PDO Exceptions when lengths of varchar are exceeded, that's a good reason to keep varchars under 255 though (possibly not this one but in general). We could open that issue again or start a new one to try to standardize on this. I'll admit to having forgotten some of that discussion when committing this to 8.x originally.

mgifford’s picture

I was talking on twitter to @dboudreau & @sgauder and Sandi said that she's been seeing alt text of up to 3 sentences lately to mirror the text imbedded in an image. Now this doesn't constitute best practice.

I think bumping this up to 500 characters is a good limit though. We should think though about using WAI-ARIA aria-describedby property.

droplet’s picture

Status: Fixed » Closed (fixed)

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

cweagans’s picture

Issue tags: +Needs backport to D7
cweagans’s picture

Issue summary: View changes

corrected verb tense