Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Has no connenction with issue 94 #95

Merged
merged 2 commits into from
Jan 21, 2019

Conversation

blacksenator
Copy link
Collaborator

@blacksenator blacksenator commented Jan 13, 2019

  • additional check wether user dslf-conf is choosen
    * delete redundant check if 'imagepath' is set in config

* delete redundant check if 'imagepath' is set in config
@@ -56,10 +56,6 @@ function uploadImages(array $vcards, $config, $configPhonebook, callable $callba
$mapFTPUIDtoFTPImageName = []; // "9e40f1f9-33df-495d-90fe-3a1e23374762" => "9e40f1f9-33df-495d-90fe-3a1e23374762_190106123906.jpg"
$timestampPostfix = substr(date("YmdHis"), 2); // timestamp, e.g., 190106123906
$configImagepath = $configPhonebook['imagepath'] ?? NULL;
if (!$configImagepath) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich sehe diesen check nicht wirklich als redundant.

Ja, inhaltlich machen die beiden Tests dasselbe. Von daher könnte man sie ggfs in eine eigene funktion auslagern, die dann von den zwei Stellen aufgerufen wird.

Der 1. Test in RunCommand macht Sinn, weil er SEHR FRÜH (vor download der vCards) auf einen späteren Showstopper hinweist. Also => Benutzerfreundlichkeit!

Der 2. Test in functions.php macht Sinn, weil der Code von uploadImages innerhalb von functions nicht sicher sein kann, ob der aufrufende Code den 1. Test vorher gemacht hat. Liegt ja außerhalb seiner Kontrolle.
Allerdings muss uploadImageses 100% wissen, sonst geht diese Funktion ja nicht. Hier geht es also um Sicherheit/Wasserdichtigkeit, wenn irgendjemand irgendwann mal den 1. Test entfernt.

=> Bitte beide Tests drinlassen.
Wenn Du es schöner machen willst, dann in eine eigene Funktion auslagern die von beiden bisherigen Stellen aufgerufen wird.

@blacksenator blacksenator changed the title no connenction with #94 Has no connenction with issue 94 Jan 14, 2019
@andig
Copy link
Owner

andig commented Jan 14, 2019

Wo kommt denn dieser dslf-conf User eigentlich her? Meine FB hat den nicht?

@andig
Copy link
Owner

andig commented Jan 17, 2019

Ping @blacksenator

@andig
Copy link
Owner

andig commented Jan 21, 2019

Ping @blacksenator was ist dieser dslf user?

@blacksenator
Copy link
Collaborator Author

"dslf-conf" is the default user in routers following this standard:
https://www.broadband-forum.org/technical/download/TR-064.pdf

For the FRITZ! Box this is the user, when logging in with a password but without username. That is why it is well suited as a default value.

According to my empirical results, this user has no ftp rights and is therefore unfortunately unsuitable for image upload.

@andig andig merged commit efe1954 into andig:master Jan 21, 2019
@andig
Copy link
Owner

andig commented Jan 21, 2019

Got it, thanks!

@blacksenator blacksenator deleted the Precondition_image_upload branch February 13, 2019 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants