Javascript error in Firefox in node edit form if uid field is not visible but author details is [#2535220]

Problem/Motivation

We add some additional fields to the node form that we put in the existing node author details element.

Normal users don't have access to change the owner/author of a node.

The nodeDetailsSummaries behavior assumes that the uid field is always visible if the wrapping details element is. In our case, that assumption is wrong.

What happens then is that name is undefined, is passed to Drupal.t(), down to formatString() and that does checkPlain(undefined), and that causes an error.

Proposed resolution

I'm not sure if this is the correct fix, but explicitly checking in Drupal.formatString() if args[key] doesn't just exist but is not undefined solves the actual JS problem. but maybe that behavior should also care about this and don't display a broken string then in that case?

Also, fun side node. seven never shows those summaries, but we still spend time on every request to calculate them them...

Remaining tasks

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#22	2535220-do-not-test.patch	544 bytes	David_Rothstein
#22
#17	2535220-17.patch	976 bytes	Sivaji_Ganesh_Jojodae
#17	7.x: PHP 5.3 & MySQL 5.5 1,990 pass, 4 fail
#12	2535220-12.patch	934 bytes	mbaynton
#12	8.0.x: PHP 5.5 & MySQL 5.5 14,425 pass
#5	2535220-node-summary-error.patch	932 bytes	droplet
#5	8.0.x: PHP 5.5 & MySQL 5.5 12,931 pass
#1	2535220-javascript-format-string-null-1.patch	461 bytes	Berdir
#1	8.0.x: PHP 5.5 & MySQL 5.5 12,928 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 17 July 2015 at 14:51

Status:

Active

» Needs review

File	Size
2535220-javascript-format-string-null-1.patch	461 bytes
8.0.x: PHP 5.5 & MySQL 5.5 12,928 pass

Comment #2

droplet CreditAttribution: droplet commented 20 July 2015 at 07:36

Firefox only?

`checkPlain(undefined)`, personally I don't think we need to handle this case, let's see what's nod_ thought.

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 20 July 2015 at 07:38

Issue tags:

+JavaScript

Comment #4

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 20 July 2015 at 08:05

To reproduce:

* admin/structure/types/manage/article/form-display, move "authored by" to hidden
* Then go to node/add/article

Yes, we've only seen it on Firefox. The problem it self should happen on Chrome as well, but for some reason, chrome never executed the problematic part:

$context.find('.node-form-author').drupalSetSummary(function (context) {
        var $authorContext = $(context);
        var name = $authorContext.find('.field-name-uid input').val();
        var date = $authorContext.find('.field-name-created input').val();
        return date ?
          Drupal.t('By @name on @date', {'@name': name, '@date': date}) :
          Drupal.t('By @name', {'@name': name});
      });

I tried adding a breakpoint inside that function but it was never triggered. Might be completely broken in Chrome but it's not visible in Seven.

Comment #5

droplet CreditAttribution: droplet commented 20 July 2015 at 08:59

File	Size
2535220-node-summary-error.patch	932 bytes
8.0.x: PHP 5.5 & MySQL 5.5 12,931 pass

Chrome has native Detail tag supports that will not trigger CollapsibleDetails.setupSummary().

Comment #6

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 20 July 2015 at 11:30

Fixing the actual problem definitely makes sense, but I think it might still be a good idea to prevent javascript errors due to incorrect custom/contrib code?

Comment #7

droplet CreditAttribution: droplet commented 10 August 2015 at 08:43

I think we should address them in 2 different issues.

Improve in Drupal.checkPlain needed further discussion.

Comment #8

droplet CreditAttribution: droplet commented 1 November 2015 at 03:24

Category:	Task	» Bug report
Issue tags:		+Needs backport to D7, +Needs manual testing, +Novice, +js-novice

Comment #9

stockholmz CreditAttribution: stockholmz as a volunteer commented 15 November 2015 at 11:57

#1 This will definitely fix this issue of not trying to handle undefined. Alternatively, to be absolutely sure you could consider checking if its a string also.

#5 This is a way better solution for the isolated bug. Although I would consider checking if undefined, rather than just bool-checking. (Unsure whether this is a code-standard thing). - Tested and confirmed to remove the error.

Comment #10

17 November 2015 at 15:07

andypost queued 5: 2535220-node-summary-error.patch for re-testing.

Comment #11

droplet CreditAttribution: droplet commented 17 November 2015 at 18:25

if statement in JS also checking `undefined` and `null`.

Comment #12

mbaynton

he/him

Minneapolis, US

CreditAttribution: mbaynton commented 23 November 2015 at 01:42

File	Size
2535220-12.patch	934 bytes
8.0.x: PHP 5.5 & MySQL 5.5 14,425 pass

reroll #5

Comment #13

mbaynton

he/him

Minneapolis, US

CreditAttribution: mbaynton commented 23 November 2015 at 02:45

Status:

Needs review

» Reviewed & tested by the community

I think this solution, #12 in particular is ready to RTBC, however wrt. #7, I agree that also checkPlain should itself guard against this to increase robustness of page's overall JS.

My reroll is just an automerge of #5; I took no actual part in authoring anything so consider myself a legit RTBC-er.

Comment #14

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 10 December 2015 at 14:57

Version:	8.0.x-dev	» 7.x-dev
Status:	Reviewed & tested by the community	» Patch (to be ported)
Issue tags:		+String change in 8.0.2

Committed da0789a and pushed to 8.0.x and 8.1.x. Thanks!

Comment #15

10 December 2015 at 14:57

alexpott committed 9b09197 on 8.1.x

Issue #2535220 by Berdir, droplet, mbaynton: Javascript error in Firefox...

Comment #16

10 December 2015 at 14:57

alexpott committed da0789a on

Issue #2535220 by Berdir, droplet, mbaynton: Javascript error in Firefox...

Comment #17

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented 27 December 2015 at 15:30

Status:

Patch (to be ported)

» Needs review

File	Size
2535220-17.patch	976 bytes
7.x: PHP 5.3 & MySQL 5.5 1,990 pass, 4 fail

Patch for D7.

Comment #18

27 December 2015 at 15:43

Status:

Needs review

» Needs work

The last submitted patch, 17: 2535220-17.patch, failed testing.

Comment #19

droplet CreditAttribution: droplet commented 28 December 2015 at 01:45

Status:

Needs work

» Reviewed & tested by the community

Comment #20

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented 1 February 2016 at 23:23

Status:

Reviewed & tested by the community

» Postponed (maintainer needs more info)

What is the bug here in Drupal 7? How is it reproducible?

The node.js file is added via this code:

  $form['author'] = array(
    '#type' => 'fieldset',
    '#access' => user_access('administer nodes'),
    '#title' => t('Authoring information'),
.....
    '#attached' => array(
      'js' => array(
        drupal_get_path('module', 'node') . '/node.js',
        array(
          'type' => 'setting',
          'data' => array('anonymous' => variable_get('anonymous', t('Anonymous'))),
        ),
.....

And then in JavaScript (as visible in the patch context):

       var name = $('.form-item-name input', context).val() || Drupal.settings.anonymous,

So how could "name" ever wind up undefined?

Comment #21

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 1 February 2016 at 23:44

Just like in D8, it's not without altering the form.

If you put custom form elements/fields into the author fieldset but the user doesn't have access to the name field, then you get this.

Comment #22

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented 2 February 2016 at 00:01

File	Size
2535220-do-not-test.patch	544 bytes

I tried that before leaving my comment - what I used to deny access to the field was the code in the attached patch. And I didn't get a JavaScript error, for the reasons mentioned above.

Now I guess there is a bug in that scenario since the summary says "By anonymous" (even though the node isn't being created by the anonymous user)! But the patch in this issue won't fix that.

Comment #23

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 2 February 2016 at 20:08

Status:

Postponed (maintainer needs more info)

» Needs work

You're right. D8 doesn't have the anonymous fallback.

I think that was removed because we switched to the default entity reference widget which uses an explicit reference for the anonymous user unlike D7. (which was then changed again and apparently this part was never changed back. But I dislike that change anyway and that's a different topic.

Anyway, you're right, I guess we can close this. Or we need an explicit check to see if the field exists and not just if it has a value or not. Because the current patch doesn't make sense.

Setting to needs work for that, but if you think that's not important enough, feel free to close it.

Comment #24

3 August 2016 at 00:04

alexpott committed 9b09197 on 8.3.x

Issue #2535220 by Berdir, droplet, mbaynton: Javascript error in Firefox...

Comment #25

3 August 2016 at 00:05

alexpott committed 9b09197 on 8.3.x

Issue #2535220 by Berdir, droplet, mbaynton: Javascript error in Firefox...

Comment #26

stefan.r CreditAttribution: stefan.r commented 28 September 2016 at 22:44

Issue tags:

-Novice

Comment #27

27 January 2017 at 17:53

alexpott committed 9b09197 on 8.4.x

Issue #2535220 by Berdir, droplet, mbaynton: Javascript error in Firefox...

Comment #28

27 January 2017 at 17:52

alexpott committed 9b09197 on 8.4.x

Issue #2535220 by Berdir, droplet, mbaynton: Javascript error in Firefox...