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

id3 tagging errors fail to be displayed #77

Closed
apotek opened this issue Nov 3, 2016 · 1 comment
Closed

id3 tagging errors fail to be displayed #77

apotek opened this issue Nov 3, 2016 · 1 comment
Assignees
Labels
Milestone

Comments

@apotek
Copy link

apotek commented Nov 3, 2016

Expected behavior

If there is an error during an id3 tagging operation (such as the one I encountered while trying to tag .wav files) I would expect to see an error coming back from the JSON call. However, the operation just silently fails.

Actual behavior

Editing song data fails silently, with no indication of failure.

Steps to reproduce the behavior

  1. Attempt to edit a file that does not support id3 such as a wav file, (or generate an error some other way).
  2. Click Save
  3. Experience the silence. Notice an error only by tailing the php log on the server, or by reloading the page and not seeing any changes to the metadata.

Possible solution (optional)

Here's the change I made to allow errors to come back to my browser during id3 operations that were failing:

In owncloud8/apps/audioplayer/controller/scannercontroller.php starting at line 421:

                        }else {
                                $result = [
                                                'status' => 'error',
                                                'msg' => (string) $tagwriter->errors,
                                        ];

changed to:

                        }else {
                                if (is_array($tagwriter->errors)) {
                                        $tagwriter->errors = implode("\n", $tagwriter->errors);
                                }
                                $result = [
                                                'status' => 'error',
                                                'msg' => (string) $tagwriter->errors,
                                        ];

I made this change since I was getting a php error on an array to string conversion on this line:
'msg' => (string) $tagwriter->errors,

Since I wasn't sure if the errors always would be arrays or just sometimes, I added this kludge. Obviously the real solution is to make sure that the type of the $tagwriter->errors is always of one kind. Since it is a plural property it would make sense to make sure that the app would always set $tagwriter->errors as an array of error messages.

Then the code above would never have to check if the errors are an array or not, and could implode() the message for browser output without the check.

Also, you might not want to use \n as the "glue" for the implode. It's was just the simplest thing I could think of.

Server configuration

Operating system:
Debian

Web server:
apache2

Database:
mysql

PHP version:
5.6

ownCloud/Nextcloud version: (see /status.php)
8.2.8

Updated from an older ownCloud/Nextcloud or fresh install:
Updated

Where did you install Audio Player from:
Via the admin interface "enable" button.

Are you using external storage, if yes which one: local/smb/sftp/...
No.

Are you using encryption: yes/no
No.

Client configuration

Operating system:
Mac OS X 10.10.5

Browser:
Firefox 49.02

Logs

ownCloud/Nextcloud log

ownCloud/Nextcloud log (`/data/[owncloud|nextcloud].log`) ``` ownCloud[19317]: {PHP} Array to string conversion at /path/obscured/owncloud8/apps/audioplayer/controller/scannercontroller.php#424 ```

Web server error log (optional)

Web server error log ``` [02/Nov/2016:19:18:39 -0400] "GET /owncloud/index.php/apps/audioplayer/editaudiofile?songFileId=5365 HTTP/1.1" 200 4673 63.138.22.162 - - [02/Nov/2016:19:18:52 -0400] "POST /owncloud/index.php/apps/audioplayer/saveaudiofiledata HTTP/1.1" 200 814 ```

Browser log (optional)

Browser log ``` Insert your browser log here ```
@Rello Rello self-assigned this Nov 7, 2016
@Rello Rello added the bug label Nov 7, 2016
Rello added a commit that referenced this issue Nov 11, 2016
Rello added a commit that referenced this issue Nov 11, 2016
@Rello Rello added this to the 1.3 milestone Nov 11, 2016
@Rello
Copy link
Owner

Rello commented Nov 11, 2016

Hello @apotek
thank you for your support.
I fixed this issue and the error message is now handed over to the user frontend (slide down warning).
it will be published with the upcoming 1.3

@Rello Rello closed this as completed Nov 11, 2016
Rello added a commit that referenced this issue Nov 11, 2016
@Rello Rello reopened this Nov 12, 2016
@ghost ghost removed the pending release label Nov 15, 2016
@ghost ghost closed this as completed Nov 15, 2016
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants