? ? Pending

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
4 Apr 2021

Code review.

This method can and is called many times with a string as the value for $button

E.g:

$bar->appendButton('Custom', $layout->render($options), 'versions');

$bar->appendButton('Standard', 'cancel', $alt, $task, false);
avatar PhilETaylor PhilETaylor - open - 4 Apr 2021
avatar PhilETaylor PhilETaylor - change - 4 Apr 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Apr 2021
Category Libraries
avatar HLeithner
HLeithner - comment - 5 Apr 2021

You can but you shouldn't because it's deprecated maybe comment this in the parameter description?

avatar PhilETaylor
PhilETaylor - comment - 5 Apr 2021

If the docblocks don't match real life then IDEs like phpStorm and static analysis like psalm will continue to error and moan.

avatar PhilETaylor PhilETaylor - change - 5 Apr 2021
Labels Added: ?
avatar ceford ceford - test_item - 5 Apr 2021 - Tested successfully
avatar ceford
ceford - comment - 5 Apr 2021

I have tested this item successfully on dec2be7

I have looked up the toolbar button code quite a bit so approve the docblock clarification that the parameter can be a ToolbarButton or a string, and the deprecation comment.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33011.

avatar richard67
richard67 - comment - 6 Apr 2021

@brianteeman I don't think we have a proper way for deprecation notes if the deprecation only concerns one of the possible data types of a mixed parameter. But I'm not sure with that. To me it seems ok like it is now in this PR.

avatar brianteeman
brianteeman - comment - 6 Apr 2021

It will be ignored by any tooling that reports on code being deprecated

avatar richard67
richard67 - comment - 6 Apr 2021

It will be ignored by any tooling that reports on code being deprecated

Yes, I saw. But do you know a way to make it ok for the tools?

avatar richard67
richard67 - comment - 6 Apr 2021

It needs a @deprecated annotation

Yes, but how is it made for a function parameter with mixed type where only one of the possible types is deprecated? I can't remember having ever seen an example for that.

avatar wilsonge
wilsonge - comment - 6 Apr 2021

I don't believe there's any deprecated annotations for parameters. You just write it in the method description (and can choose to use the @note annotation as well) - at least to the best of my knowledge. Not sure where the latest developments on the PSR-5 saga have got to...


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33011.

avatar richard67 richard67 - test_item - 7 Apr 2021 - Tested successfully
avatar richard67
richard67 - comment - 7 Apr 2021

I have tested this item successfully on 35f71bf


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33011.

avatar richard67 richard67 - alter_testresult - 7 Apr 2021 - ceford: Tested successfully
avatar richard67 richard67 - change - 7 Apr 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 7 Apr 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33011.

avatar richard67
richard67 - comment - 7 Apr 2021

Previous test by @ceford was still valid since last change after that was just a change on the deprecation comment. It has a correct annotation now so can be found by tools.

avatar HLeithner
HLeithner - comment - 8 Apr 2021

if you use string now you get a deprecation, if you read the documentation you don't see any deprecation. So I would simple add the note to the parameter or what george suggested use the @note annotation please?

avatar rdeutz
rdeutz - comment - 14 Apr 2021

@PhilETaylor could you add a note and we are good to merge this, thanks.

avatar PhilETaylor
PhilETaylor - comment - 15 Apr 2021

Using @note will not achieve the aim of this PR which is to have the deprecation shown in IDE and static analysis tools. Those tools need the @deprecated tag.

avatar SharkyKZ SharkyKZ - test_item - 15 Apr 2021 - Tested unsuccessfully
avatar SharkyKZ
SharkyKZ - comment - 15 Apr 2021

I have tested this item ? unsuccessfully on 35f71bf

This method is not deprecated.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33011.

avatar PhilETaylor PhilETaylor - close - 15 Apr 2021
avatar PhilETaylor PhilETaylor - change - 15 Apr 2021
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2021-04-15 15:45:09
Closed_By PhilETaylor
Labels Added: ?

Add a Comment

Login with GitHub to post a comment