[TYPO3-core] RFC: #10853: [Feature] New Multimedia CE

[Steffen Kamper]

Hi Benni,

Benjamin Mack schrieb:
> Hey,
> 
> Here is my review for v17 (dude, 17 versions? ;-)).
>
thx! The big amount of versions is because i raised the numbers while 
developing, and big task needs a lot of (re)coding and enhances.


> ===== Linespaces =====
> 
> As Franz Koch mentioned, there are still some whitespace changes.
> 
> -$incFile = $GLOBALS['TSFE']->tmpl->getFileName($filename);
> -if ($incFile)    {
> -    $fileinfo = t3lib_div::split_fileref($incFile);
> +$incFile = $GLOBALS['TSFE']->tmpl->getFileName($filename);
> +if ($incFile)    {
> +    $fileinfo = t3lib_div::split_fileref($incFile);
> 
> 
> I think he's referrring to the added lines ADD linespaces AFTER the ; on 
> this line.
>

i already searchd for and will eleminate the others too.

> 
> ===== helper functions =====
> 
> I love "readFlexformIntoConf", this could be used for so many of my 
> plugins. However, I don't like that it is merged into a parameter.
> 
> I would rather have a return value that is conf, and then we can merge 
> the result with the $conf var. right?
> 
>   $ffConf = $this->flexform2array($flexData);
>   $conf = array_merge_recursive($ffConf, $conf);
> 
> 

yeah, i also thought about general usage of this function. So making it 
global and not merge inside is a good idea, i will update this.


> +     * @param    boolean        is set if called recursive. Don't call 
> function with this parameter, it's used inside the function only
> +     */
> +    function readFlexformIntoConf($flexData, &$conf, $recursive=0) {
> +        if ($recursive === 0) {
> 
> Well, the parameter $recursive is "0" by default, although it's boolean. 
> Then you have a $recursive === 0 (which is a type comparison as well), 
> so this is either wrongly coded or wrongly documented :)
>

yes, that's bad, i update.

> What about "arrayKeyMap"? This should not stay in tslib_content as it's 
> very generic. I'd also rename it, as array_map() is something else :). 
> "remapArrayKeys" or something like that, in t3lib_div.
> 
> 

true. It's a generic array function and can be useful for others too. As 
long as we don't have a t3lib_array, t3lib_div is the best place. I will 
move it and rename as you suggested.

> ====== TS constants =======
> 
> +    # cat=content/cMedia; type=int+; label= Video Media Width: define 
> the default width for the media-object   
> +  videoPlayer = typo3/contrib/flashmedia/flvplayer.swf
> 
> +    # cat=content/cMedia; type=int+; label= Audio Media Width: define 
> the default width for the media-object
> +  audioPlayer = typo3/contrib/flashmedia/player.swf
> 
> 
> Define the default width of the player?
>

good one

> ====== MediaItems Hook ======
> 
> +if (is_array 
> ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['tslib/hooks/class.tx_cms_mediaitems.php']['customMediaRender'])) 
> {
> +    foreach 
> ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['tslib/hooks/class.tx_cms_mediaitems.php']['customMediaRender'] 
> as $classRef) {
> +        $hookObj = &t3lib_div::getUserObj($classRef);
> +        $conf['file'] = $url;
> +        $content = $hookObj->customMediaRender($renderType, $conf);
> +    }
> +}
> 
> Please add the $mode to the $conf variable as well.
> 
> 

i thought it's in conf already, i will proove it.

> ==============
> 
> Apart from that, the latest patch (v17) does not apply anymore to 
> current trunk!
> 
> 

sure. I come with updated patch taking your comments into account 
tomorrow. Thx for taking your time.

vg Steffen