Page MenuHomePhabricator

mw.html Scribunto library methods such as css, cssText and attr should not generate errors when passed a nil value
Closed, ResolvedPublic

Description

Author: hlmzgy

Description:
After inserting the nil argument to cssText() function in the HTML library, an exception "Invalid CSS Given: Must be Either a string or a number." is thrown. Strangely, this bug is not present in the [[:en:Module:HtmlBuilder]] (cf. [//en.wikipedia.org/w/index.php?title=Module:HtmlBuilder/testcases&diff=600781294&oldid=573508575 testcases]).


Version: unspecified
Severity: normal

Details

Reference
bz62982

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 2:55 AM
bzimport set Reference to bz62982.
bzimport added a subscriber: Unknown Object (MLST).
bzimport created this task.Mar 23 2014, 3:17 PM

Is there a minimal, self-contained testcase for this?

Note that [[:en:Module:HtmlBuilder]] doesn't do a lot of error checking that it probably should. I'm inclined to say that this error is entirely appropriate (and therefore this bug INVALID), since nil is not valid CSS.

hoo added a comment.Mar 24 2014, 4:19 PM

This is expected behavior which we built in on purpose. You should check your arguments before passing them.

When a user passes a nil value specifically, the intended (and useful) behavior would be a no-op, to avoid filling code with "if(foo.bar) baz:cssText(foo.bar) end" constructs. See https://en.wikipedia.org/wiki/Module_talk:Message_box#Protected_edit_request_on_9_April_2014 for more. I don't see any benefit in throwing an error rather than a no-op in this case.

Why was this re-WONTFIXed?

hoo added a comment.Apr 21 2014, 8:03 PM

Because "This is expected behavior": nil is not a valid css string!

That doesn't make it expected. For nil, expected behavior is a no-op, just like ordinarily appending a nil element to a table is.

(In reply to Jackmcbarn from comment #7)

That doesn't make it expected. For nil, expected behavior is a no-op, just
like ordinarily appending a nil element to a table is.

Agreed - not silently accepting nil feels unexpected, mostly because it kills call-chaining. (Personally I would also like false values to be silently ignored, as it makes it easier to use the "and" and "or" operators when call-chaining, but I can understand if others feel this is too permissive.)

hoo added a comment.Apr 22 2014, 11:23 AM

I guess I can be made to agree with such changes if there is a wider consensus for such things AND if all mw.html functions act in a consistent manner. It's not acceptable that some take nil values etc. while others don't.

Yes, I meant to say that this should work consistently across all the mw.html methods, not just cssText. I've changed the summary of this bug accordingly. Also, I'll start a discussion on enwiki's Lua project page to get some more opinions.

Discussion link: [[Wikipedia talk:Lua#mw.html library nil behaviour]].

Would anyone support a configuration flag?
It could be set as mw.html.acceptNilCSS = false
I would suggest keeping it true by default, just for the sake of call chaining, but it could also be set for a single element only, as:
el = mw.html.create('div'); el.acceptNilCSS = false
Just in those cases the library will throw an error, otherwise it won't do anything.

That seems like unnecessary complexity. What's the benefit?

(In reply to Jackmcbarn from comment #13)

That seems like unnecessary complexity. What's the benefit?

I would rather support comment #0, but not everyone agrees...

(In reply to Ricordisamoa from comment #14)

I would rather support comment #0, but not everyone agrees...

comment #7, of course.

  • Bug 66166 has been marked as a duplicate of this bug. ***

Change 137659 had a related patch set uploaded by Jackmcbarn:
Allow passing nils to mw.html

https://gerrit.wikimedia.org/r/137659

Change 137659 merged by jenkins-bot:
Allow passing nils to mw.html

https://gerrit.wikimedia.org/r/137659