When allowing a "title" with an image field, the textfield element allows a maxsize of 500. However, the default column size is VARCHAR(128). If someone types more than 128 characters into the field you end up getting the "an unknown error has occurred" message.

This patch gives a default maxlength of 128.

On a related note, there are variables for the maxlength of the alt and title fields, but nowhere to set them through the UI. Would it be worth it to add that config to the field/instance settings forms?

Thx!

CommentFileSizeAuthor
#130 1015916-tests.patch1.26 KBxjm
#126 title_field_allows_too_many_chars-1015916-126-D7.patch3.05 KBtheborg
#125 title_field_allows_too_many_chars-1015916-125.patch2.5 KBtheborg
#123 title_field_allows_too_many_chars-1015916-123.patch1.24 KBjenlampton
#123 title_field_allows_too_many_chars-1015916-123-D7.patch1.8 KBjenlampton
#117 title_field_allows_too_many_chars-1015916-116-D7.patch1.77 KBjenlampton
#114 title_field_allows_too_many_chars-1015916-114.patch1.23 KBjenlampton
#114 title_field_allows_too_many_chars-1015916-114-D7.patch1.78 KBjenlampton
#109 title_field_allows_too_many_chars-1015916-109.patch1.23 KBjenlampton
#107 title_field_allows_too_many_chars-1015916-107-D8.patch1.23 KBjenlampton
#107 title_field_allows_too_many_chars-1015916-107-D7.patch1.78 KBjenlampton
#103 title_field_allows_too_many_chars-d8-1015916-103.patch2.09 KBjenlampton
#103 title_field_allows_too_many_chars-d7-1015916-103.patch2.04 KBjenlampton
#102 add_var_get_for_db_column_size-1015916-102.patch2.09 KBjenlampton
#97 1015916_97.patch1.3 KBBTMash
#89 1015916_d7.patch3.12 KBBTMash
#86 1015916-title-allows-more-than-database-stores-86.patch1.95 KBrealityloop
#82 1015916_82.patch1.92 KBBTMash
#79 1015916_79.patch2.7 KBBTMash
#68 1015916_imagefield_flexible_title_alt_varchar.patch4.24 KBchristefano
#59 imagefield_flexible_title_alt_varchar_1015916_d8_59.patch4.11 KBBTMash
#55 imagefield_flexible_title_alt_varchar_1015916_d8-55.patch3.05 KBAron Novak
#40 imagefield_flexible_title_alt_varchar_1015916_d8_40.patch3.47 KBBTMash
#26 imagefield_flexible_title_alt_varchar_1015916_d8_26.patch2.54 KBBTMash
#21 imagefield_flexible_title_alt_varchar_1015916_d8_21.patch766 bytesBTMash
#9 imagefield_flexible_title_alt_varchar_1015916_9.patch2.65 KBBTMash
#8 imagefield_flexible_title_alt_varchar_1015916_8.patch2.18 KBBTMash
#4 imagefield_flexible_title_alt_varchar_1015916_4.patch757 bytesBTMash
image_title_maxlength.patch838 bytesKevin Hankens
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Component: field system » file.module

recategorizing

anruether’s picture

Glad that someone looked at this issue! Kevin, is it possible to VARCHAR to 500 as it is supposed by the textfield? Would be useful to have more than just 128 characters in some cases!

BTMash’s picture

Agreed, I think the varchar being set to a higher value would be better. I'll attach a patch for making the field a variable length.

BTMash’s picture

I'm attaching a patch for increasing the field length to be whatever variable_get('image_title_length', 500) would be. Not sure on how to approach image_update_7001(), however.

BTMash’s picture

Status: Active » Needs review

Forgot to mark as 'needs review'.

joachim’s picture

Status: Needs review » Needs work

There's no sane way to set a database field length based on a variable...

Also, is there even a UI to set the image_title_length variable?

Seems to me we either:

- limit the UI to 128 (bit short)
- increase the DB size and also add a limit on the UI
- make the DB size the same as node body, that is, effectively unlimited. which is a bit crazy.

yched’s picture

@BTMash : "Not sure on how to approach image_update_7001(), however"

Hint : see how text_update_7000() does.

BTMash’s picture

Status: Needs work » Needs review
FileSize
2.18 KB

Thank you for the hint, @yched. I'm attaching a revised patch which has update_7001. I'm not touching the UI aspect of things since I'm unsure on what direction this should go in (other than agreeing that 128 is too short and blog is perhaps too long). I'm revising the title to be '500' and the alt to be '80' (both in quotes since its being set to whatever is in the variable_get() of those two variables.

BTMash’s picture

Hmm, I didn't realize there was already an update_7001 in trunk :) Attaching new patch. This patch was creating against git so hopefully there won't be any issues :/

BTMash’s picture

Regarding the ui and/or how this should be handled...i'm not sure. I don't know if having users fill out how long they want the title/alt text field to be for a field type makes a lot of sense (one set of image fields would have a 80 char limit on the title while others would have a 500 char limit on the title? it would be confusing for a content editor when filling out this data). From what I see on the original imagefield in contrib, it had a data column which was of type text (so I'm going to guess a very large field) - at the same time, it could also hold other kinds of data in there (it held both the alt and title fields though I can't recall what else...). I think a hardcoded size (128 is a bit short...512 is reasonably long) would be the better solution than the other two. My second option would be same size as node body (which would be overkill) with the settings in the ui being the third and really least preferred option.

anruether’s picture

Thanks for your work BTMash, I haven't tested the patch, but will do so.

Concering your question: From a user's point of view it would be a very good solution to take the same value as in d6, 500 or 512. In some cases this length is really needed, but I never heard that people are writing novels in the picture's title ;)

And yes, for a content editor, it would be a bit confusing to be confronted with details like that, i think. On the other hand, who for some people it would be of advantage to be able to set the value, as long as it a setting in a not too prominent place...

So, if you don't mind the extra work, why not put the option somewhere in the menus (i'm not sure where it would belong to) with a reasonable standard value. otherwise setting something like 500 would also be fine for 99% of users as far as I see.

BTMash’s picture

First off, thank you for the reply, anruether.

I agree that 512 is good ('reasonably long' might have sounded like I am against it but I'm all for it) and *might* debate it being something like 1024 characters (or longer). I don't know if I really agree with a settings page to set these 2 values (the closest place where the setting might make sense it on the image toolkit page? I'm not entirely sure) and thus will try to see if its possible to just include this as part of the field setting (have it default to 512 but let the user change it if need be). I'm hoping for some guidance from others in the community on where this setting makes sense if there is going to be a UI option for users to change the value.

anruether’s picture

Yes, I completely agree! I didn't mean to set up an extra settings page, but, yes, I also thought about the image toolkit page or the field setting page.

Thans again for your work!

TripX’s picture

Same problem for me. Has anyone tested this patch? Will it be included then in the drupal 7.1?

BTMash’s picture

Version: 7.x-dev » 8.x-dev

I'm upping this to an 8.x issue since things have to be backported down to 7. Will have to see where things are with 8 and possibly re-roll the patch.

@tripx, I am actually running this patch on one of my sites. With that said, I don't think my voice should count since I'm one of the people that wrote out the patch ^_~

kehan’s picture

subscribing

mmgg’s picture

Sorry but I'm confused here..

I use the title field for image captions, some which can be a bit long (over 128 char for sure) and am having problems with drupal 7, giving me errors with long captions.

Will this patch allow that? and does #15's comment about it being backported mean its coming to D7 soon????

Help?!

BTMash’s picture

The patch that is above IS for d7. However, since I didn't realize that D8 patches need to be created first, get committed there, and then port the d7 version of the patch for usage.

BTMash’s picture

Status: Needs review » Needs work

Forgot to chance status

matrobin’s picture

subscribe

BTMash’s picture

Ok, this is a first-round patch for Drupal 8. The backport is already above and that code has been modifed to fit what is now up for d8. Things that need to be done about the patch.

1) Decide on what the length of the fields will be. Right now, I have left them at the default of whatever Drupal marks them (80 for the alt text, 500 for the title). We need to change this.
2) If we decide it should be flexible, I need the help from someone else on how to alter columns in a table. I found the form that does this but I need guidance from the drupal-core seniors on how to approach this.
3) If we hardcode this, it should be a reasonably high value (maybe 500 chars for both? 1024? I can't be the one to make the call on this).

Once that is decided, then we can make a more effective patch (and backporting anything that is missing should be much simpler).

BTMash’s picture

Status: Needs work » Needs review

arg...forgot to mark as 'needs review'

joachim’s picture

Status: Needs review » Needs work

I don't think storing a system-wide variable for this is the right thing at all -- especially as there's no UI to set those variables.

If we want it flexible, then this should be stored per-field, and there's already a system for a field having a bunch of options. hook_field_schema receives the $field as a parameter, so that probably contains all the options -- do a debug to find out.

However... I'm not sure about adding this as an option. How many users will actually know what to choose when confronted with this? When building a site, how many of us know how long the image title field may turn out to be? Best thing here is a sane default, I think.

BTMash’s picture

I had changed the field schema - I wasn't sure on how to make it on a per-field basis (or on a global basis and where that setting would go and/or how to retrieve it).

Agreed - the fact that there is no ui to change this makes it quite baffling on why there are/were variables for this in the first place. And having some sane default seems like the best solution to me as well (though requests to make it longer might make it more..problematic down the line?) I've been reading up regarding this issue online along with taking a look at what imagefield did. Imagefield had one column called 'data' which was a large block that contained serialized data; obviously that wreaks havoc on actually doing interesting things with that data without unserializing it (can't do queries to get particular fields via views, for example).

Would there be an issue with making the title a text field and making the alt field something fairly large like 512/1024 chars? I know with the alt, there is more of an issue on which browsers support it...

mmgg’s picture

I agree a UI for this option would confuse a lot of users and doesn't seem necessary.
Simply changing max title length to 1024 would solve all of my problems, im assuming other's as well.

BTMash’s picture

Status: Needs work » Needs review
FileSize
2.54 KB

New revision of patch. Got rid of the system settings variables and replaced with a constant definition (both alt and title are set to 1024).

Status: Needs review » Needs work

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

BTMash’s picture

Hmm...the failure makes no sense in the context of this patch. There is a failure in search.test (which I don't get on my machine and I just ran 'git pull').

BTMash’s picture

Status: Needs work » Needs review
BTMash’s picture

Given that the patch passed the automated testing, would anyone else be able to review the patch?

BTMash’s picture

Assigned: Unassigned » BTMash

I've created the backport for this as well but I'm not sure of the protocol for putting it up (since it would go through the simpletests and I assume fail?). Should the backport patch be posted up after the 8.x version of the issue is resolved?

ddiakopoulos’s picture

Assigned: BTMash » Unassigned

Any movement on this issue? BTMash's patch sufficiently satisfies the problem; the 1024 length seems more than adequate, even if a little long. It would be nice to see this in core.

MarkFischer’s picture

I've tested the #26 patch on a stock Drupal 7.2 install and can confirm that it functions as designed. Both the alt and title database fields are now set to varchar fields with 1024 lengths. The maxlength attributes on the edit forms is also properly set to match at 1024.

MarkFischer’s picture

Status: Needs review » Reviewed & tested by the community
aspilicious’s picture

Is this only for 8.x? Doesn't this need an upgrade path. And why 1024 it is a lot bigger than any other textfield in core.

BTMash’s picture

@aspilicious the 7.x branch will need the upgrade path. D8 should not require it (since D8 has not yet released). I'm using 1024 as that should be enough room for people to add whatever title/alt text they need; my own needs are for 512 but one or two folk mentioned. With that said, it should be easier to figure out where changes need to be made.

christefano’s picture

I just saw BTMash apply this patch during a meetup and it was awesome!

aspilicious’s picture

Issue tags: +Needs backport to D7

tags

webchick’s picture

Status: Reviewed & tested by the community » Needs work

If you're changing the schema, you need to add an update hook. Additionally, you can't specify a constant in the .module file and use it in the .install file, because .modules are not loaded at install time.

Is 1024 actually necessary though? That's a significant jump from 128.

BTMash’s picture

Status: Needs work » Needs review
FileSize
3.47 KB

I have added in an update hook (starting at 8000 - for D7, it will be at 7002 as far as I can tell right now?). I have also changed it back to using the variable_get() functions but setting the default value to be 1024 in them.

As I stated above, my own needs are for 512 but there were a couple of folk in here that stated 1024 as something suiting their needs.

Edit: Correction - one person. But I digress.

MarkFischer’s picture

I'd definitely like the 1024 length. I'll try the new version of the patch this week hopefully.

dreadlocks1221’s picture

I'm actually in need of something like this, is the patch ready to be used on 7.7 or do I need to wait for more testing? I too would be in favour of a full 1024 length.

BTMash’s picture

I'm currently using it on one of my sites and its working well. It *does* need more testing in that it would be great to get this in core :)

sashdo’s picture

Thanks a lot!

And Plz, could you provide also tar.gz file for dummies, to make it possible to apply the patch without linux/unix environment? I would realy appreciate it, unfortunately I am not a programmer and do quiver with fear if I have to open Cygwin and to do something in it :)

dreadlocks1221’s picture

I Installed the patch yesterday, seems to be working well enough. Hope it gets moved to core at some point

Sashdo, you can install netbeans ide, just open the file and under the tools there is a diif compare option you can use to apply patch

sashdo’s picture

Thank you dreadlocks1221.
Frankly I tried to install netbeans, but in my PC environment I am even unable to configure it. There are so many podcasts and manuals for Mac users, even very dummy-oriented, but I can´t figure it out for PC Netbeans 7.0, I have VERY different interface in my version and simply have no options and menus they say I need to configure.
Without proper config, after trying to apply a patch it says that the patch was "partially applied", it changes only 2 lines of original file and as a result the altered module still limits the field to 128 characters.
It seems to me I do need a true dummy solution :-). Sorry...

dreadlocks1221’s picture

Its going to say it only applied it partially because that patch is for two files not one (you need to also patch the install file)

sashdo’s picture

Both files were patched, I have seen their names in the patch file and patched both. But no way. :( That´s why I thought that "partially" refers to something that could not be patched in one of the files.
Anyway, thank you a lot for your advice and participation. I will have to work with this site for a long time, it´s a long-term project, so I do hope that someday I will be smart enough to figure out what´s going wrong...

BTMash’s picture

dkh’s picture

Hi, thanks for this patch.

Is there any estimate on when this will be backported to D7? I just joined drupal.org. Let me know if there is anything I can do testing wise to try to help the process.

BTMash’s picture

@dkh, the patches first need to pass against Drupal 8. Once it is accepted and committed to Drupal 8, the patch is then backported to Drupal 7 (in this case, the backport should essentially be the same). Only at that point would it get in there. Currently, the patch still needs to get tested and reviewed by others (I wrote the patch so I am obviously going to have bias if I update the status to 'reviewed and tested by the community' and thus cannot do so). So help with reviewing the code for the patch and testing it out to ensure it actually works would be *very* helpful.

Aron Novak’s picture

Status: Needs review » Needs work

I'm wondering if adding the variable_get() into the .install file makes sense. It won't adapt anyways to the variable value changes, so i don't see what it solves. Why not simply use fixed value of 1024 in .install file and let the variable in the .inc file, like the original version. Site builders can shorten the allowed length according to their needs. If they make it 2048, it's their problem. And it's not possible to handle anyways in a simple way.

BTMash’s picture

I'm a bit confused by what you mean. The original file also used the variable_get. Based on what webchick said, using constants was not a good idea. Atleast this way, if the variable is changed via the variable table, future tables will have the same change reflected appropriately. I'm also not fully understanding how site builders would be able to shorten it without changing the code. There are several places where the changes need to get made in order for something like this to come in place.

Aron Novak’s picture

"The original file also used the variable_get"
Not in the .install file:

-        'length' => 128,
+        'length' => variable_get('image_alt_length', 1024),

"I'm also not fully understanding how site builders would be able to shorten it"
For example one trivial way, installing the devel module, go to devel/php and execute a variable_set. OR editing settings.php: http://drupaleasy.com/quicktips/module-development-settings-variables-se...

Aron Novak’s picture

To be a bit more constructive, here is my version of the patch. With a bit simplified update hook.

Aron Novak’s picture

Status: Needs work » Needs review

@BTMash: sorry, i failed to understand what's going on here. #40 should be fine, as changing the variable does not affect already existing fields, right? If you like, you can include the update hook simplification from #55.

BTMash’s picture

@Aron, thanks for the clarification. I think setting it to be hardcoded in the schema but not in the module leads to the problem that a change in variable (via variable_set from devel) does not mean that schema will change. The patch I wrote *would* allow for that change (atleast for any future fields). So someone could go back to their image field and pending that it is empty could allow for the schema to change whereas your patch would still mean that the module install file has to get patched. I would prefer having a variable (or constant defined that could be used everywhere easily as opposed to changes in multiple areas.

Aron Novak’s picture

I have another idea in mind. I think the patch would be pretty mutch RTBC if it would allow the admins to change it (in a not user friendly way, but still)
So i'd add this to default.settings.php

/**
 * Image module title and alt field length configurations:
 *
 * In the image module, by default title and alt are
 * 1024 characters long. By configuring it, you can
 * adjust the maximal lengths for those image field
 * instances what will be created in the future.
 *
 */
# $conf['image_alt_length'] = 1024;
# $conf['image_title_length'] = 1024;

For example #1055276: Configurable chunk size in drupal_http_request() does the same. There is a faceless variable, configurable via settings.php, with comments explained.

BTMash’s picture

That is a good idea. I have created a patch with your additions to the settings.php file.

Lars Toomre’s picture

Issue tags: +D8 upgrade path

Since this patch has an update hook, its landing will need to be delayed until #1182290: Add boilerplate upgrade path tests lands first.

BTMash’s picture

Thinking this through...why *does* the D8 version need an upgrade path? I know webchick asked for it to be in here. But if D8 is still in dev and assuming that this would be backported into D7 (or is that a false assumption?), I'm confused about the requirement. Would it mean that a site that comes up later in D8 would still get this upgrade going through even though it may have done so in the D7 site?

ccoppen’s picture

How do we get backports of the current patches? This particular one is something I need immediately as this is an annoying bug. Or, how can I create a personal backport myself? Is there a process for that?

BTMash’s picture

@ccoppen Most of the patch should drop in relatively simply. The image_update_8000 line would become image_update_7002 (or 700N) - there is a way to do it using schema_alter as well though I'm not entirely familiar with it. Getting this RTBC would however mean it would be able to get into D8 (and D7) core much more quickly, however.

BTMash’s picture

ultimateboy’s picture

Component: file.module » image.module

While I definitely agree that the 128 char limit should be bumped up (and I'm in the 512 camp. I think 1028 is crazy), I do not believe that this should be configurable. Whether or not this setting uses variable_get and/or defaults set in settings.php should probably be in another issue. I'd like to see this issue trimmed down to simply increasing the size of the field. Let's get that in D8, backported to D7 and then worry about making things configurable for D8 in an alternate issue. Thats my two cents.

BTMash’s picture

@ultimateboy, what you said is what the patch currently does and was decided on a while ago.

BTMash’s picture

Issue tags: -D8 upgrade path

Now that #1182290: Add boilerplate upgrade path tests is in core, anyone care to review this one to try and get in as well? :)

christefano’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.24 KB

This is ready to go. I also changed "tool tip" to "tooltip" to bring it in sync with the rest of core.

chx’s picture

Status: Reviewed & tested by the community » Closed (duplicate)
catch’s picture

Status: Closed (duplicate) » Postponed

I'm going to keep this open because we need to track the individual places this is happening (unless we come up with something generic but that hasn't happened yet and may well not). However marking it postponed on the other issue.

BTMash’s picture

Hmm, is there anything else that can be done regarding the columns? Changing it to be a TEXT field would alleviate a lot of issues.

BTMash’s picture

I should have mentioned then that the schema for alt/title become text fields. In the module themselves, there would a limit on the number of chars that a user could enter.

catch’s picture

Damien Tournoud’s picture

Status: Postponed » Needs work

Let's fix this.

varchar columns are portable up to a limit of 4000 characters (I thought we had this documented somewhere, but apparently not).

If this is deemed sufficient (I think there are industry-accepted limits on the number of characters in those fields), let's stick with varchar, else move to text.

Also, image_update_8000() needs to only touch the fields that are stored using field_sql_storage.

BTMash’s picture

My understanding from looking about is that MySQL < 5.0.3 has a limit of 255 characters on varchar (mariadb, sqlite, sql server, postgresql, db2, oracle all support more than 255 characters - not sure on drizzle or any other dbs out there) based on what I see at http://dev.mysql.com/doc/refman/5.0/en/char.html. So it would cause an issue on people with older versions of MySQL. Not sure on how much of an issue this is, however.

Regarding the fields...I thought that was taken care of with:

+  $fields = _update_7000_field_read_fields(array(
+    'module' => 'image',
+    'storage_type' => 'field_sql_storage',
+  ));

as that should only allow for updating fields that are using sql storage.

catch’s picture

dasjo’s picture

BTMash’s picture

Coming from a hiatus and checking back in. Just looked at #1297598: Raise minimum MySQL version requirement to 5.0.3 and I guess I'm trying to understand exactly what would be a roadblock on this issue now given that the minimum version of MySQL supported by D7/D8 supports the longer varchar lengths (and pretty much all the other DB engines are also good about long varchar lengths).

Mind you, this patch, if it looks good, relies on #1182296: Add tests for 7.0->7.x upgrade path (random test failures) to come through so this can be properly tested.

BTMash’s picture

FileSize
2.7 KB

I was looking at the patches for the D8 version of #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O and I am now a bit puzzled as to why the update hook needs to be implemented for the D8 version (as it was not implemented in any of those patches) and this issue has been tagged with needing a backport to D7 (which would have the update hook). I'm re-rolling the patch to be up to date with HEAD but without the update hook.

As for the character limit, any ideas on what the path regarding that should be would be greatly appreciated.

BTMash’s picture

Status: Needs work » Needs review

Should have set to 'needs review'

catch’s picture

There's no barrier to long varchars now afaik, and yeah 8.x patches should not have updates if we reasonably expect them to be backported to 7.x. The variable_get() for column length is a no go though - if you change the variable the schema isn't going to change so we should just pick one.

BTMash’s picture

FileSize
1.92 KB

Under such a scenario, I see 2 possible options then:

  1. Hard code a length and wholly get rid of the image_title_length and image_alt_length variables since they don't offer anything other than a possible way to end up with PDOExceptions.
  2. Convert the alt and text columns from varchar into TEXT (and other sql engine equivalent) columns. Much higher limit on text length. Makes more sense to use the above variables in this scenario.

Attaching patch with option (1) for now. I have been working with a max length of 1024 which is reflected in the patch.

catch’s picture

Are those variables used for form validation/max length? If so we could always do min(1024, $variable) in those places - then they can be less but never more than storage.

TEXT columns cause temporary tables to be written to disk rather than memory, so we need to be very careful to avoid that if this is the case with image fields.

BTMash’s picture

The variables are strictly used for maxlength (from what I see, they do not come up anywhere else in the module) so I'm ok with that type of solution (shall I reroll it then with that in mind?). Or should we worry about it. I imagine that if someone wants more than 1024 characters, they would do form and schema alters (though I am unfamiliar with schema alters on fields) And agreed on the text columns (it was the reason I did not rewrite the patch in that direction.). One final question would be: is 1024 enough? I know its been asked a few times but just want anyone that needs more to chime in now than later.

Status: Needs review » Needs work

The last submitted patch, 1015916_82.patch, failed testing.

realityloop’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

rerolled for /core

BTMash’s picture

theamoeba’s picture

I have tested the patch in #86 everything works perfectly.
Please note that I had to reinstall (uninstall/install) the Image module before the changes worked.
An update hook needs to be added.

BTMash’s picture

FileSize
3.12 KB

Since this fix also needs to get into D7, I am adding the fix to that. Attaching patch for D7.

jenlampton’s picture

Status: Needs review » Needs work

We shouldn't be using varcar for columns longer than 255. If we *really* need these columns to be 1024, they should be changed to text.

Also, if we're really getting rid of the variables image_title_length and image_alt_length entirely, they should be removed in the update hook for D7 as well.

jbrown’s picture

jbrown’s picture

varchars can be up to 65,535 in D7 (MySQL 5). Column type TEXT is heavyweight and should be avoided.

jenlampton’s picture

I don't think text is heavyweight, the only difference is that you can't index a table on a text field. But, I'm not sure I could see a reason to index this table on title or alt anyway. (and that's not entirely true about varchars - that's assuming it's the only varcar column in the table that are NOT NULL, and in this case we have at least 2 that are)

My point was actually that I think 1024 is overkill for the size of this field. :) How about 255?

BTMash’s picture

The varchar length issue(s) have come up in #74, #81 , and #83. And the general outcome has been long varchars are ok and text is not feasible (I don't mind them being text fields if it goes in that direction though). A few folk wanted atleast 1024 chars (I had a need for 512 and so 255 does not cut it. The D6 imagefield module allowed for longer text allowed in the field and doing upgrades / migrations caused some bad errors.

jbrown’s picture

http://dev.mysql.com/doc/refman/5.6/en/blob.html

Each BLOB or TEXT value is represented internally by a separately allocated object. This is in contrast to all other data types, for which storage is allocated once per column when the table is opened.

and see #83

theamoeba’s picture

Why must the image title field be so long? Shouldn't an optional description/caption field be added for this purpose?
It seems ridiculous to have a title of 1024 characters.

BTMash’s picture

FileSize
1.3 KB

This is utterly pointless. The issue has gone bouncing around for over 10 months (on what were effectively changes to 4 lines of code) and we're back at square one on why 1024 characters, etc. I've started writing out a contrib module that resolves this issue but in order for it to work (and surprising, contrib cannot solve it because field schema cannot actually be altered before creation; you can change it but updates to a field may remove data), I'm moving efforts into #691932: Add hook_field_schema_alter() which hopefully has better chances of resolve for users and how they want to use the fields than this.

Last patch effectively returns back to the patch in #1 but we increase the title length and decrease the alt length to be 128 characters (same as what is in the schema). This needs a backport to D7. Atleast no more PDO exceptions.

jbrown’s picture

There is no disadvantage to the max being 1024. Make the change in #91 and delete the variables in the update hook and I'll RTBC this.

webchick’s picture

I'm +1 for #97. Let's just fix the bug. If we want to discuss increasing the size of the text field, let's do it in a separate feature request that does not hold up a bug fix.

webchick’s picture

Status: Needs work » Needs review

Marking needs review.

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, bug fix first :) The patch in #97 works for me. Nice validation fail with too much text in alt.

for increasing the length of both fields see #1353030: Increase length of "alt" and "title" text for images.

jenlampton’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.09 KB

On second thought, if we leave these values as variables here we're still going to be running up against the same issue as soon as someone increases the size of the variable beyond what the database can actually hold.

Attached patch uses variable_get on the database column size too. This will allow those who want more room to be able to use this variable to increase the size of their columns as well.

jenlampton’s picture

Status: Needs review » Needs work

The last submitted patch, title_field_allows_too_many_chars-d7-1015916-103.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review

Silly test bot. that's a D7 patch!
/me goes and reviews patch naming conventions for d7 vs d8

BTMash’s picture

Status: Needs review » Needs work

As per #81, using variable_get would be a no-go. But if it is, the D7 version of the patch requires a reroll for an update hook. Removal of the variable (along with an update hook that does variable_del) would be the way to go.

jenlampton’s picture

Thanks @BTMash :)

Trying again - removing variables from both versions entirely, and adding an update hook for the D7 version too.

jenlampton’s picture

Test bot?

jenlampton’s picture

Status: Needs work » Needs review
FileSize
1.23 KB

I'll get it eventually :)

jbrown’s picture

There is currently a full stop at the end of the url which makes it invalid - I think there should be a space before the full stop.

// See http://www.gawds.org/show.php?contentid=28.

should be

// See http://www.gawds.org/show.php?contentid=28 .
jenlampton’s picture

Or we could just leave off the period. How about:

// @see http://www.gawds.org/show.php?contentid=28
jbrown’s picture

Yeah - better with no full stop / period.

Do we use doxygen keywords in inline comments?

aspilicious’s picture

No you can't use @see inline :).

jenlampton’s picture

Doh. Okay, trying with the space before the period.

theborg’s picture

Tested 8.x version and is working ok on FF and Chrome.

jbrown’s picture

Status: Needs review » Reviewed & tested by the community
jenlampton’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.77 KB

Working for me too. Can someone else test the upgrade path for D7? I *think* its working but would like to have confirmation

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

whoopsie.

theborg’s picture

Working on D7. RTBC.

xjm’s picture

For the comment, I think it should be moved to the line above, and use @see rather than just "See"? That way, it also doesn't need to have that pesky period. :)

xjm’s picture

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

Also, we should add an automated test for this.

jenlampton’s picture

I'm gonna need help with the tests :/

For the comment on the line above, do you mean like this?

  $element['alt'] = array(
    '#title' => t('Alternate text'),
    '#type' => 'textfield',
     '#default_value' => isset($item['alt']) ? $item['alt'] : '',
     '#description' => t('This text will be used by screen readers, search engines, or when the image cannot be loaded.'),
    /*
     * @See http://www.gawds.org/show.php?contentid=28
     */
    '#maxlength' => 128, 
     '#weight' => -2,
     '#access' => (bool) $item['fid'] && $settings['alt_field'],
   );
jenlampton’s picture

after conferring with @xjm in IRC we found 52 instances of // @see in core. Re-rolled with updated comments.
...but I still need help with the tests.

theborg’s picture

D8 patch working, for the test we can add something like:

    $image_info = array(
      'uri' => $node->{$field_name}[LANGUAGE_NONE][0]['uri'],
      'alt' => $this->randomName(2000),
      'title' => $this->randomName(2000),

to image.test file and then check for the system rejecting entered values

theborg’s picture

Added a test for d8 patch.

theborg’s picture

BTMash’s picture

The patches look very good to me (someone else will need to rtbc it since the core has not deviated from the original patch I wrote).

BTMash’s picture

Removing the 'needs tests' and 'backport' tags.

BTMash’s picture

Issue tags: +Needs backport to D7

I think I might have been trigger happy on removing things. Keeping the 'needs backport' since it needs to come down to D7.

xjm’s picture

FileSize
1.26 KB

Here's a patch with just the tests to expose their failure without the fix.

Status: Needs review » Needs work

The last submitted patch, 1015916-tests.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

No PDO error, form works as anticipated. Woot! I'm marking this RTBC :)

However, I think there maybe a usability issue (generally) with now #maxlength works on larger form elements. I'm going to take that up over here though #1360466: Remove maxlength HTML attribute from form elements where #maxlength is >= 128, as not to hold up this issue :)

BTMash’s picture

+1 on rtbc :)

dotman’s picture

just to confirm, which patch is tested and ready to be applied for D7? also, new to patching core. could i get some guidance or a reference? on osx if that matters.

thanks.

xjm’s picture

@dotman: Try the patch in #126. Instructions for applying a patch with git can be found in the second half of this handbook page: http://drupal.org/node/707484. You can use either git apply patchname.patch (if it is a git repository) or patch -p1 < patchname.patch with the file in the root of your Drupal installation.

dotman’s picture

great thank you. what would be the command to reverse the patch if things break? I will definitely test it.

thanks.

dotman’s picture

ran patch # 126 on a local install of d7.7 and it seemed to apply:

patching file modules/image/image.field.inc
Hunk #1 succeeded at 358 (offset -45 lines).
Hunk #2 succeeded at 368 (offset -45 lines).
patching file modules/image/image.install
Hunk #1 succeeded at 244 (offset -142 lines).
patching file modules/image/image.test
Hunk #1 succeeded at 778 (offset -8 lines).

But I still get the pdo exception error. I did flush the cache and ran the update script. am I missing something? character count was between 500-550 or so. Does this patch have to applied before an install?

really need this functionality. Is there a known working patch for D7? I didn't see the one I tried had been tested, but I may be wrong.

thanks.

dotman’s picture

i ran the patch this time before a doing fresh install, and I noticed the title filed did truncate my text, whereas on an existing install, it didn't, but did give the pdo exception. so it did make a change, but won't save without the error. any help of course is appreciated.

thanks.

jenlampton’s picture

@dotman this issue has to be fixed in Drupal 8 before it will get fixed in Drupal 7. This patch still only allows 128 characters to be saved into the database, but should make the form element truncate the text for you.
Also See #1353030: Increase length of "alt" and "title" text for images.

dotman’s picture

i see. my mistake. looking forward to the patch. any idea on timeline? thanks for all of your and others work here.

thanks.

jenlampton’s picture

@dotman soon, I hope. This D8 patch is marked RTBC, which means it could be committed any day :) As soon as that gets committed we'll move this issue back to D7 and start the testing process (and you can help! :) The -D7 patch in #126 should still apply cleanly to Drupal 7.10 (no promises about 7.7) so that's probably where we'll start.

mike stewart’s picture

+1 tested #126 on D8. works as expected. other thoughts:

más uno
أكثر من واحد
פּלוס איינער
συν ένα
plus én

... just sayin

( tested at LGB contribute meetup: http://groups.drupal.org/node/191758 )

mike stewart’s picture

doh! re-read the issue... and thought it would make more sense to test #126 against Drupal7!

so tested against current D7.10 and D7.x -- still, all good.

so patch #126 worked as expected on D7 & D8

catch’s picture

Version: 8.x-dev » 7.x-dev

Lookgs good, committed/pushed #125 to D8. Looks like #126 has the patch for 7.x so leaving at RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, that works.

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -Needs backport to D7

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