Well, this is less than minor but... the schema description is wrong for the {cache_field} table, since it says:

Generic cache table for caching things not separated out into their own tables. Contributed modules may also use this to store cached items.

This comes from the direct copy of the generic {cache} table at line 166 of field.install:

  $schema['cache_field'] = drupal_get_schema_unprocessed('system', 'cache');

Comments

LaurentAjdnik’s picture

Assigned: Unassigned » LaurentAjdnik
Status: Active » Needs review
Issue tags: +Quick fix

The description itself might need to be rewritten:

  $schema['cache_field']['description'] = 'Cache table for the Field module to store already built field informations.';
LaurentAjdnik’s picture

StatusFileSize
new475 bytes
swentel’s picture

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

Moving to 8.x, care to re-roll ? :)

swentel’s picture

Issue tags: -Quick fix

Status: Needs review » Needs work
Issue tags: +Quick fix

The last submitted patch, 892344.field_.install.cache_description.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
StatusFileSize
new473 bytes

Here's a reroll

ramlev’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and working

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Committed/pushed to 8.x, moving to 7.x for backport. The 7.x backport should technically have an update function to correct the schema definition for existing sites.

tstoeckler’s picture

I'm not a native speaker, hence, not resetting this directly, but I'm pretty sure "informations" is not proper English. In that case we should roll a short follow-up to fix that typo before backporting this.

dcam’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -Quick fix, -Needs backport to D7
dcam’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs backport to D7, +Needs reroll, +Novice

The "informations" text has been removed from D8, so this no longer needs a follow-up.

#2 does need to be rerolled for D7 though. As mentioned in #8, D7 will also need an update function.

amitgoyal’s picture

Status: Needs work » Needs review
StatusFileSize
new787 bytes
new762 bytes

Please review update patch as per fixes in #8.

brahmjeet789’s picture

In drupal 7.x-28, I have not seen any cache field description in field.install file but in patch #12 it includes cache description field in that file.

dcam’s picture

Assigned: LaurentAjdnik » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

I'm going to mark #12 as RTBC, but with reservations. I don't think we should be using SQL queries in update functions. Unfortunately, I don't see any means in the Database API to alter a table's comment. David_Rothstein will need to weigh in on it.

Otherwise, the patch works as promised. The field_cache table description is set to the new text on install and on update.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

mgifford’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 892344-field_.install.cache_description-12.patch, failed testing.

David_Rothstein’s picture

This at least fails for SQLite (I tested); have a feeling it will fail for PostgreSQL too. Also, the update function should be inside the "updates-7.x-extra" block, not outside...

I'm not really sure what to do about this if there isn't a way to add a table comment in a cross-database-compatible way. Is it so bad to fix this for new installs only, and skip the update function?

hussainweb’s picture

Status: Needs work » Needs review
StatusFileSize
new452 bytes

The database issue is non-functional. In my opinion, it doesn't seem too bad to just skip the database update. I have attached a patch, which is basically the same as the patch in #6.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

I agree it's fairly pointless to worry about an incorrect schema description on existing D7 sites at this point.

dcam’s picture

Agreed. I thought about this while travelling today and decided I would rather see the change get in for new sites than let this issue sit in limbo forever because the update isn't supported by the database API. RTBC +1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: wrong_schema-892344-39.patch, failed testing.

Status: Needs work » Needs review
mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: wrong_schema-892344-39.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: wrong_schema-892344-39.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

dcam queued 39: wrong_schema-892344-39.patch for re-testing.

tstoeckler’s picture

StatusFileSize
new452 bytes

That's a random fail. Re-uploading to keep the context of the fail.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

Status: Fixed » Closed (fixed)

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