[TYPO3-core] RFC #9615: Move frontend editing to a system extension

[Jeff Segars]

Hey Steffen,
Getting back around to updating the patch and addressing these comments. 
  Updated patch attached.

Steffen Kamper wrote:
> class.t3lib_frontendedit.diff
> =============================
> 
> 
> +            if(is_callable(array($this, $cmd))) {
> +                $this->$cmd($table, $uid);
> +            }
> would be helpful to call an exception if action isn't available

I'm fine with adding that.  Do we have any standards in place already 
about where the exception classes live or anything like that? I hate to 
pollute t3lib with another class that doesn't do much of anything.

> $hideField = $GLOBALS['TCA'][$table]['ctrl']['enablecolumns']['disabled'];
> 
> before there was a check on $hideField, now there is no check. This 
> cause a problem with
> $recData[$table][$uid][$hideField] = 1;
> when $GLOBALS['TCA'][$table]['ctrl']['enablecolumns']['disabled'] is 
> NULL, is this filled all the time?

The code here is actually the same as before unless I'm overlooking 
something.  Pasting it below...

> public function hide($table, $uid) {
> 	$hideField = $GLOBALS['TCA'][$table]['ctrl']['enablecolumns']['disabled'];
> 	if ($hideField) {
> 		$recData = array();		
> 		$recData[$table][$uid][$hideField] = 1;
> 		$this->tce->start($recData, array());
> 		$this->tce->process_datamap();
> 	}
> }


> strtolower($conf['allow']
> 
> should be t3lib_div::strtolower($conf['allow'] for turkish reasons

In this case, allow is a list of actions that come from a predefined 
list (show, edit, etc) so Turkish shouldn't really be a concern. I went 
ahead and changed it just in case someone defines their own action though.

> There are some CGL-issues, the tabbed comments and some spaces missing 
> after comma.

Scrubbed the file again for everything I saw.  Comments are actually 
supposed to be tabbed until we have an official decision on that and an 
update to the CGL, right?

  > class.tx_feedit_adminpanel.diff
> ================================
> 
> $row .= '<img src="' . TYPO3_mainDir . 'gfx/ol/' . ...
> 
> please use scinning interface (and CGL)

I'm wondering if the skinning interface actually makes sense right now. 
The call to setFancyDesign() ignores skinning so changing icons based on 
the current skin could actually break things and at a minimum will cause 
an inconsistent look and feel.

Not saying skinning support shouldn't be added, but its probably outside 
the scope of this patch that is just for refactoring.

>  . chr(10)
> just annotation - would be better to have global constant LINE_BREAK

If you're OK with it, I'll leave it as-is for now since the chr(10) is 
used only 3 times in adminpanel and once in editpanel. If there was 
already a global value, I could see using it but the usage is rare and I 
don't see it as something that will be changing.

> function getHiddenFields, foreach($val as $k => $v)    {
> 
> please change to foreach($val as $valKey => $value)    {
> 
> 
> .. some CGL like $out.=

Scrubbed again for CGL.


> while(list(,$row)=each($GLOBALS['BE_USER']->extPageInTreeInfo)) {
> 
> please use foreach
> 
> +            reset($GLOBALS['BE_USER']->extPageInTreeInfo);
> +            while(list(,$row) = 
> each($GLOBALS['BE_USER']->extPageInTreeInfo)) {
> 
> the same

Done :)


> function extGetHead: the scinning again.
> 
> function extItemLink: CGL
> 
> also the tabbed comments

Scrubbed again for CGL.  Same comment as above on the tabbed comments :)

> class.tx_feedit_editpanel.diff
> ===============================
> 
> +            $content = (strlen($cBuf) && $securCount) ? 
> substr($content, 0, strlen($cBuf)) . $icon . substr($content, 
> strlen($cBuf)) : $content = $icon . $content;
> 
> this looks like a problem in utf8-mode with the native strlen, substr etc.

Are you OK with addressing this in a separate RFC? From a quick glance, 
it looks like some brittle code to me and I'd rather give any changes 
the full attention of a normal RFC rather than rolling it into a big 
patch where it gets lost.

> $cf1 = "if (confirm(" . t3lib_div::quoteJSvalue($confirm, true) . ")){";
> 
> should be single quotes

Done.

> just a joke in between:
> $tceforms->setFancyDesign() sounds like an easteregg :D

Yeah, I always thought that one was funny too, given that the design 
isn't particularly fancy :)

Thanks for the review and testing!
Jeff

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009615v2.diff
Type: application/pgp-keys
Size: 171094 bytes
Desc: not available
Url : http://lists.netfielders.de/pipermail/typo3-team-core/attachments/20081103/f4fb1fa4/attachment-0001.key