As suggested in #1893800-47: Something is very, very wrong with update.php / upgrade tests... demons suspected I filed separate issue

I got random number of failed upgrade tests in http://qa.drupal.org/pifr/test/443928
Suppose this random failures are caused by undocumented usage of displays in upgrade from #1852966-126: Rework entity display settings around EntityDisplay config entity but not sure

The number of failed upgrade tests that fails are always different also I can't reproduce it locally in PHP 5.3.14

CommentFileSizeAuthor
#19 get_t_doc_fix-1916556-19.patch570 bytesaroq
#16 get_t_doc_fix-1916556-16.patch581 bytesaroq
#12 doc.fix-1916556-12.patch577 bytesaroq
#10 doc.fix-1916556.patch550 bytesaroq
#5 doc.patch578 bytesandypost
#4 interdiff.txt8.63 KBandypost
#1 sandbox-comment.patch364.99 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Title: Random test failures - » Random test failures - The string cannot be saved
Status: Active » Needs review
FileSize
sandbox-comment.patch364.99 KB

The code from drupal.org/sandbox/larowlan/1790736 branch comment-fieldapi4
Attached patch from current version

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Priority: Major » Normal
Status: Needs review » Postponed

Postponed for new bugs

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +Random test failure
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
interdiff.txt8.63 KB

And again... after my try to minimize a key length according #1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL
Same error http://qa.drupal.org/pifr/test/445168

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Priority: Normal » Major
Status: Postponed » Needs review
FileSize
doc.patch578 bytes

After digging into this problem with jthorson we found that we have no ability to use get_t() within hook_update_N() see #1907960-61: Helper issue for "Comment field" and #68 where this was fixed.

So simple we should change docs for hook_update_N() to avoid this confusion.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Component: other » update.module
Issue tags: +D8MI

Tagging

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
CreditAttribution: podarok commented
Status: Needs review » Reviewed & tested by the community

#5 simple
looks good

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Component: update.module » database update system

FWIW the "update.module" is specifically about the Update Manager (previously Update Status), while "database update system" is about hook_update_N(), update.php and friends.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

We don't need to point out it's broken now, instead we should update the docs of get_t() to indicate that it shouldn't be used during code run in updates.

aroq’s picture

aroq CreditAttribution: aroq commented
Status: Needs work » Needs review
FileSize
doc.fix-1916556.patch550 bytes

Patch attached to update get_t() docs as noted at #9.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Thanks @aroq!

Two minor notes on the patch:

  1. We don't normally use contractions in our docs, so "shouldn't" should probably be "should not".
  2. hook_update_N should be followed by parens: hook_update_N().

I have another question: What should be used in place of get_t()? We should provide that information as well, I think.

aroq’s picture

aroq CreditAttribution: aroq commented
FileSize
doc.fix-1916556-12.patch577 bytes

Updated patch as noted in #11.

Status: Needs review » Needs work
Issue tags: -D8MI, -Random test failure

The last submitted patch, doc.fix-1916556-12.patch, failed testing.

aroq’s picture

aroq CreditAttribution: aroq commented
Status: Needs work » Needs review

#12: doc.fix-1916556-12.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI, +Random test failure

The last submitted patch, doc.fix-1916556-12.patch, failed testing.

aroq’s picture

aroq CreditAttribution: aroq commented
FileSize
get_t_doc_fix-1916556-16.patch581 bytes

Updated patch attached.

aroq’s picture

aroq CreditAttribution: aroq commented
Status: Needs work » Needs review

Issue status updated.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

We removed the word please from the UI, I guess we shouldn't use it in docs either? So just "use t() instead".

aroq’s picture

aroq CreditAttribution: aroq commented
Status: Needs work » Needs review
FileSize
get_t_doc_fix-1916556-19.patch570 bytes

Updated as said at #18.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Not 100% sure if t() is really what you should use but hook_update_N() says so, although we also have example that don't translate at all in core. @catch should probably look at this.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: Unassigned » catch

Moving over to him, per Berdir.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

I think we should change the docs in hook_update_N() and not use any of these there - same as other API functions that potentially invoke hooks etc.

dcam’s picture

dcam CreditAttribution: dcam commented
Issue tags: +Needs issue summary update

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Priority: Major » Normal

OMG no, don't tell people to use t() in update functions, that could very well be a death spell too. The db behind t() changes in the D8 update process for example. #1813762: Introduce unified interfaces, use dependency injection for interface translation may or may not help with that at the end.

Downgrading because I have not seen this appearing anytime recently as well as because the suggested solution is a one line comment expansion.

jair’s picture

jair CreditAttribution: jair commented
Issue tags: +Needs reroll

Needs reroll

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Closed (won't fix)

get_t() doesn't exist anymore.

https://drupal.org/node/2021435