Bug #38998

Task #38984: Code Review

Fix of Phoenix Package code reviews

Added by Tizian Schmidlin over 9 years ago. Updated over 9 years ago.

Status:
Closed
Priority:
Must have
Category:
TYPO3 5.x
Start date:
2012-07-16
Due date:
% Done:

100%

Estimated time:
5.00 h
Spent time:

Description

Alle Punkte von Tizians Code Review übernommen. Durch die Umstrukturierungen im Code wurden die geänderten Funktionen nochmal kurz getestet.

#1

Updated by Ivan no-lastname-given over 9 years ago

  • Status changed from New to Closed
#2

Updated by Ivan no-lastname-given over 9 years ago

Nachfolgend noch alle Bemerkungen aus dem Codereview von Tizian:

Remarks by Tizian Schmidlin on 2012-07-16 15:05:
Please change the way you get the information about the files extensions and protocols
so that you can set it in the Settings.yaml file. Also rename the variables so that
they comply to the coding guidelines (lowerCamelCase).

Remark by Tizian Schmidlin on 2012-07-16 15:16
This should be extended with the possibility to pass an array
as $value and every value is checked. Then true is only return if a value of the
array matches. Else false is returned.

Remark by Tizian Schmidlin on 2012-07-16 15:18
Should pass an array of values to the startsWith method, else https links will be
prepended with http://, i.e. https://www.google.ch will result in beeing
http://https://www.google.ch.

Remark by Tizian Schmidlin on 2012-07-16 15:15
(Optional) Replace this by substr($url, -1,1), this should be faster.

Remark by Tizian Schmidlin on 2012-07-16 15:23
Most probably this has never created any problem before because the randomized files have
always been random but as it is here, file_exists($file) will check if the file with the
name stored in $file exists in the root of the system (which will always return true).
So the while condition should check the whole path (as returned by the method).

Remark by Tizian Schmidlin 2012-07-16 15:43
Spellcheck the name of the method and replace it everywhere.

Remark by Tizian Schmidlin 2012-07-16 15:46
Shouldn't the url be set as it was stored in the session if the request doesn't
apply to this instance?

Remark by Tizian Schmidlin on 2012-07-16 15:33
Before base64_decode the value should be passed through str_replace(' ','+',$v),
this lead to an issue in another part of the extension before.

Remark by Tizian Schmidlin 2012-07-16 15:48
This should be handled in the entirely in the method getExtensionHtmlOutput so that you
only set the value to $htmlOutput. Also you should store the value of $htmlOutput in the
session in getExtensionHtmlOutput.

Remark by Tizian Schmidlin 2012-07-16 15:50
Please add {} around the if statement.

#3

Updated by Tizian Schmidlin over 9 years ago

  • % Done changed from 0 to 100

Also available in: Atom PDF