[#3588] - [#33430] Cleaning up datalist for JFormfieldText
- Closed
- 31 May 2014
- Medium
- Build: staging
- # 3588
- Diff
- Bakual:Datalist
- Foreign ID 33430
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Issue
There is a phpcs issue with the signature of JHtmlSelect::suggestionlist(). It contains a required $idtag after the optional $optkey and $optText, which essentially makes those two required as well.
Fixing this correct is basically not possible without creating a new function and proxy it.
The produced datalist also has some errors in it.
Solution
- This PR will deprecate
JHtmlSelect::suggestionlist()which was introduced with J3.2 to implement the<datalist>tag into the text formfield. - The datalist will be created directly in the
getInput()function of the formfield instead. It's a very simple code and the current usage ofJHtmlSelect::options()withinJHtmlSelect::suggestionlist()to generate the items created more problems and was basically overkill. - Deprecated
getSuggestions()in the text formfield and replaced it withgetOptions()like we use in every other formfield.getSuggestions()is proxied togetOptions()in case someone was extending the class and actually used it.
Testing
Add some options to a text field and see if the datalist works. For example change the title for the article form to
<field name="title" type="text" label="JGLOBAL_TITLE"
description="JFIELD_TITLE_DESC"
class="input-xxlarge input-large-text"
size="40"
required="true"
>
<option>Option1</option>
<option value="Value2">Option2</option>
<option value="Value3" />
</field>
It should show a list with "suggestions" as soon as you start typing or click into the field.
Please note that the first option will not show because there is no value given. The second option will show with both the value and the text, depending on browser (Chrome shows the text rigth aligned a bit smaller). The third option will show the value only.
Remarks
This is based on a Codestyle PR and some discussions we had after it.
Unfortunately I forgot to do the PR then and now can't find the discussion anymore.
Tracker
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33430
Note
Same as #3246 but against staging and rebased.
| Title |
|
||||||
Indeed, in this case it would break. Do you think we can do it in a BC way?
In practice I don't think anyone was overriding getSuggestions. They rather wrote their own field because the datalist is currently buggy anyway.
Worst case would be that the suggestions don't show. So even if someone overrode the method, the impact would be minimal.
I don't necessarily see the need to change the method name tbh. Maybe we just mark it as deprecated but say it will become getOptions in J4
I mean you're right I dunno whether that is a use case in which case we should be safe.
I don't necessarily see the need to change the method name tbh. Maybe we just mark it as deprecated but say it will become getOptions in J4
There are two reasons: First one is that it's the standard method name we use in other fields to get the options. But more importantly was that it allows to show a deprecation message in case someone uses the old method name.
I could deprecate it without a message. For that it doesn't really matter if I rename it or not as I could load getSuggestions which would be remapped to getOptions like I do already.
So the question is what's better. Deprecating without a log message or taking a tiny BC but showing a message.
I guess according to our new strategy, not showing a message is the only option.
My prefererred option is that we can mark as deprecated without moving to
the new function immediately
On 13 May 2014 13:23, Thomas Hunziker notifications@github.com wrote:
I don't necessarily see the need to change the method name tbh. Maybe we
just mark it as deprecated but say it will become getOptions in J4There are two reasons: First one is that it's the standard method name we
use in other fields to get the options. But more importantly was that it
allows to show a deprecation message in case someone uses the old method
name.I could deprecate it without a message. For that it doesn't really matter
if I rename it or not as I could load getSuggestions which would be
remapped to getOptions like I do already.So the question is what's better. Deprecating without a log message or
taking a tiny BC but showing a message.
I guess according to our new strategy, not showing a message is the only
option.—
Reply to this email directly or view it on GitHub#3588 (comment)
.
My prefererred option is that we can mark as deprecated without moving to the new function immediately
I've removed the log message and reverted to use getSuggestions() for now to achieve full backward compatibility.
The deprecated getSuggestions() will still proxy to the new getOptions(). This allows extensions developers to change their extended fields ahead of time.
phproberto
- | Description | <h3>Issue</h3> <p>There is a phpcs issue with the signature of <code>JHtmlSelect::suggestionlist()</code>. It contains a required <code>$idtag</code> after the optional <code>$optkey</code> and <code>$optText</code>, which essentially makes those two required as well.</p> <p>Fixing this correct is basically not possible without creating a new function and proxy it.</p> <p>The produced datalist also has some errors in it.</p> <h3>Solution</h3> <ul> <li>This PR will deprecate <code>JHtmlSelect::suggestionlist()</code> which was introduced with J3.2 to implement the <code><datalist></code> tag into the text formfield.</li> <li>The datalist will be created directly in the <code>getInput()</code> function of the formfield instead. It's a very simple code and the current usage of <code>JHtmlSelect::options()</code> within <code>JHtmlSelect::suggestionlist()</code> to generate the items created more problems and was basically overkill.</li> <li>Deprecated <code>getSuggestions()</code> in the text formfield and replaced it with <code>getOptions()</code> like we use in every other formfield. <code>getSuggestions()</code> is proxied to <code>getOptions()</code> in case someone was extending the class and actually used it.</li> </ul><h3>Testing</h3> <p>Add some options to a text field and see if the datalist works. For example change the title for the article form to</p> <pre><code> <field name="title" type="text" label="JGLOBAL_TITLE" description="JFIELD_TITLE_DESC" class="input-xxlarge input-large-text" size="40" required="true" > <option>Option1</option> <option value="Value2">Option2</option> <option value="Value3" /> </field> </code></pre> <p>It should show a list with "suggestions" as soon as you start typing or click into the field.<br> Please note that the first option will not show because there is no value given. The second option will show with both the value and the text, depending on browser (Chrome shows the text rigth aligned a bit smaller). The third option will show the value only.</p> <h4>Remarks</h4> <p>This is based on a Codestyle PR and some discussions we had after it.<br> Unfortunately I forgot to do the PR then and now can't find the discussion anymore.</p> <h4>Tracker</h4> <p><a href="http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33430">http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33430</a></p> <h4>Note</h4> <p>Same as <a href="https://github.com/joomla/joomla-cms/pull/3246" class="issue-link" title="Cleaning up datalist for JFormfieldText">#3246</a> but against staging and rebased.</p> | ⇒ | <h3>Issue</h3> <p>There is a phpcs issue with the signature of <code>JHtmlSelect::suggestionlist()</code>. It contains a required <code>$idtag</code> after the optional <code>$optkey</code> and <code>$optText</code>, which essentially makes those two required as well.</p> <p>Fixing this correct is basically not possible without creating a new function and proxy it.</p> <p>The produced datalist also has some errors in it.</p> <h3>Solution</h3> <ul class="task-list"> <li>This PR will deprecate <code>JHtmlSelect::suggestionlist()</code> which was introduced with J3.2 to implement the <code><datalist></code> tag into the text formfield.</li> <li>The datalist will be created directly in the <code>getInput()</code> function of the formfield instead. It's a very simple code and the current usage of <code>JHtmlSelect::options()</code> within <code>JHtmlSelect::suggestionlist()</code> to generate the items created more problems and was basically overkill.</li> <li>Deprecated <code>getSuggestions()</code> in the text formfield and replaced it with <code>getOptions()</code> like we use in every other formfield. <code>getSuggestions()</code> is proxied to <code>getOptions()</code> in case someone was extending the class and actually used it.</li> </ul><h3>Testing</h3> <p>Add some options to a text field and see if the datalist works. For example change the title for the article form to</p> <pre><code> <field name="title" type="text" label="JGLOBAL_TITLE" description="JFIELD_TITLE_DESC" class="input-xxlarge input-large-text" size="40" required="true" > <option>Option1</option> <option value="Value2">Option2</option> <option value="Value3" /> </field> </code></pre> <p>It should show a list with "suggestions" as soon as you start typing or click into the field.<br> Please note that the first option will not show because there is no value given. The second option will show with both the value and the text, depending on browser (Chrome shows the text rigth aligned a bit smaller). The third option will show the value only.</p> <h4>Remarks</h4> <p>This is based on a Codestyle PR and some discussions we had after it.<br> Unfortunately I forgot to do the PR then and now can't find the discussion anymore.</p> <h4>Tracker</h4> <p><a href="http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33430">http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33430</a></p> <h4>Note</h4> <p>Same as <a href="https://github.com/joomla/joomla-cms/pull/3246" class="issue-link" title="Cleaning up datalist for JFormfieldText">#3246</a> but against staging and rebased.</p> |
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-05-31 08:26:13 |
I don't think this is B/C because if anyone extends the form field and overrides the getSuggestions() method - in the past the options list would have been edited - with this patch this will now be broken because people would need to override the getOptions method.