Last modified: 2014-06-19 16:57:32 UTC
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]).
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.
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?
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.)
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
