[TYPO3-core] RFC: #9591: Integrate sys_actions into toolbar

[Ingo Renner]

Oliver Hader wrote:

Hi Olly,

thanks for taking the time to review the patch!

> Line 10: please use single quotes

fixed

> Line 71: should be protected instead of private

arguable as this is not meant to be modified or returned to anybody 
other than the class itself. Changed to protected nevertheless.

> Line 87: missing @return

why? there's no return in the ethod...

> Line 88: missing type for TYPO3backend

fixed

> Line 102: isn't "if ($actionEntries)" enough?

right, fixed

> Line 107: I could not find anything about concatenation over multiple
> lines - but I think the "." should be at the end of each line

I checked this by randomly picking some files, and also found the dots 
at the end of the lines... fixed

> Line 123: would it be possible to use a string instead of an array?

How? BTW, a string is returned already, did I get this wrong?

> Line 162: should be "is_resource($queryResource)"

this would then contradict you request for removing the empty() further 
up ;)

> Line 174: after the while the TYPO3_DB->sql_free_result(...) is missing

thanks! added

> Line 218: what is this method for if it always returns true?

it's required by the backend_toolbarItem interface. It's usefull/used 
f.e. with/in the clear caches menu I think.


> Line 265ff: a dimension of "0px" can be written "0" in CSS without a unit

yeah, but that's not clean IMHO.

> Line 331: Please use "SysactionMenu" instead of "sysactionMenu"

changed to SysActionMenu

> Line 398: Hm... is it possible to have more instances of this toolbar
> item? But I'm fine with it - just a question...

it probably is, but wouldn't make a lot of sense...


please find the revised patch attached


best
Ingo

-- 
Ingo Renner
TYPO3 Core Developer, Release Manager TYPO3 4.2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: sysaction_menu_v3.diff
Type: text/x-diff
Size: 12144 bytes
Desc: not available
Url : http://lists.netfielders.de/pipermail/typo3-team-core/attachments/20081020/52476227/attachment-0001.diff