search

ampache / ampache

A web based audio/video streaming application and file manager allowing you to access your music & videos from anywhere, using almost any internet enabled device.
http://ampache.org
GNU Affero General Public License v3.0
3.55k stars 591 forks source link

source-changes, bugs and errors #2574

Closed lachlan-00 closed 3 years ago

lachlan-00 commented 4 years ago

i'll be commenting with issues and bugs that i find over the course of using source-changes branch so we can start working towards moving this branch into develop

todo

upnp errors

mpd is okay but i have a few issues with upnp, need to check more players but getting a lot of JS issues trying to start playback

seems to be an issue of adding tracks to play

Uncaught Error: Syntax error, unrecognized expression: UPnP instance = moode
...................STATE = STOPPED
Play next
The next song failed to start
    jQuery 7
    reloadDivUtil https://music.org/lib/javascript/base.js:145
    jQuery 4
Uncaught Error: Syntax error, unrecognized expression: UPnP backend disabled..
XML Parsing Error: syntax error
Location: https://music.org/server/ajax.server.php?page=localplay&action=command&command=skip&id=2
Line Number 1, Column 1: ajax.server.php:1:1
XML Parsing Error: syntax error
Location: https://music.org/index.php#localplay.php?action=show_playlist
Line Number 1, Column 1:

questions

errors i've grepped to check

resolved

lachlan-00 commented 4 years ago

init on login page possibly?

AH01071: Got error 'PHP message: PHP Fatal error:  Uncaught Error: Call to a member function format() on string in /var/www/ampache/src/Config/Init/InitializationHandlerGlobals.php:43\nStack trace:\n#0 /var/www/ampache/src/Config/Init/Init.php(55): Ampache\\Config\\Init\\InitializationHandlerGlobals->init()\n#1 /var/www/ampache/src/Config/Init.php(37): Ampache\\Config\\Init\\Init->init()\n#2 /var/www/ampache/public/index.php(30): require('/var/www/ampach...')\n#3 {main}\n  thrown in /var/www/ampache/src/Config/Init/InitializationHandlerGlobals.php on line 43'
usox commented 4 years ago

init on login page possibly?

Yep, but atm I have no idea where it comes from. The behaviour shouldn't have changed, so this will be a nice little hunt...

lachlan-00 commented 4 years ago

writing a new config doesn't work, just blanked my config on the last update image

lachlan-00 commented 4 years ago

need to work out why scrutinizer isn't working as well https://scrutinizer-ci.com/g/ampache/ampache/?branch=source-changes

lachlan-00 commented 4 years ago

I'm about ready to jump over, there are a few little things i want to do before i'm 'feature complete' but not much!

usox commented 4 years ago

The last big commit comes today or tomorrow. After merging it, the detail work will begin...

lachlan-00 commented 4 years ago

Now that I understand the browse class a bit better I'm going to work on private catalogues as my last big thing for develop this year

usox commented 4 years ago

Keeping source-changes in sync with develop takes a lot of time - so I'm happy when this ends...

usox commented 4 years ago

@lachlan-00 Ok, I did merge 6 weeks of work into source-changes. Two of the most important changes:

lachlan-00 commented 4 years ago

I'll be finishing off my Catalog work today then I think it's time to go all in for source-changes.

I was cleaning up code but I think I'll stop that so you don't have to keep moving between both.

wagnered commented 4 years ago

@usox

I was going to create a PR for this, but it would probably much easier to make the change youself if you haven't already made the change.

InstallationHelper.php:641 the path needs an additional set of double dots (/../../..).

usox commented 4 years ago

@wagnered Thanks, fixed it. @lachlan-00 Writing the new config should work again

lachlan-00 commented 4 years ago

i'll test it out now

lachlan-00 commented 4 years ago

will also need these checks updated for the install image

I'll be doing a db update for my catalog change and i'll test a config file update as well soon

lachlan-00 commented 4 years ago

and downloading the config includes html on the bottom image

lachlan-00 commented 4 years ago

i'm going to work in edge again (86577fe0f) for a bit while i finish off this catalog filtering.

it needs a dbupdate as well as a cache table to keep the catalogs filtered. might change a few things before i finish back in develop. probably this week at least now to finish but this will give source-changes a bit more time.

the update.php doesn't seem to be able to update the database either, i'm assuming there isn't an action for it yet? 2020-11-18T20:38:07+10:00 [] (Ampache\Module\Application\ApplicationRunner) -> Found handler Ampache\Module\Application\Update\ShowAction for action ``

wagnered commented 4 years ago

I finally got my local source-changes branch in sync with upstream/source-changes. I hope it is now in sync with your local branch.

Maybe you have already taken care of the follwing if so, "Never mind".

Anyway, in InstallationApplication:198 the full path of the file being passed to $this->installationHelper->install_rewrite_rules is /var/www/ampache/src/Application/../../public/channel/.htaccess but installationHelper is in /var/www/ampache/src/Module/System/. Needless to say it fails installation.

I'm having a look at using $_SERVER['DOCUMENT_ROOT'] instead of __DIR__ to get an absolute file path to the config files.

usox commented 4 years ago

@wagnered Thanks, I fixed those paths.

Regarding your suggestion: I think we should avoid bringing in some magic globals in here because this would create an implicit dependency - the functionality would depend on $_SERVER, so any other usages (e.g. cli) would become a problem. Maybe, we could build some kind of PathProvider which internally deals with lookups to certain locations instead. Depending on the usage scenario (sapi, cli, ...) they could use those globals to provide the lookup functionality.

But in my opinion, those solutions will come up automatically when those locations will be further refactored. The InstallationHelper for example is just an intermediate solution to group and encapsulate functionality - the class itself is way too big and still untestable atm. If we reach the point to be able to unit-test this class and its functionality, most of the DIR calls will be covered by tests and won't be a problem any longer :)

usox commented 4 years ago

@lachlan-00

and downloading the config includes html on the bottom

Should be fixed. If not, please let me know the action which is invoked there. Also the path-problem in the screenshot above should me gone by now.

wagnered commented 4 years ago

@usox

I'll again preface this with: If you already fixed the following then Never mind.

  1. I start off with a clean browser cache and do a fresh install. I got:

    Uncaught Error: Call to a member function format() on string in /var/www/ampache/src/Config/Init/InitializationHandlerGlobals.php:43

added alias to use Ampache\Module\System\Core as SysCore; and used SysCore to create the class object.

  1. Was then able to install. After install and update, went to login page. Upon logging in, got blank page. I then set URL to fetch localhost:4000/index.php and went to Ampache's landing page. This is repeatable, if browser cache is deleted each time, otherwise might not be noticed. I left to it to you to solve😇.

  2. This, for some reason didn't show up on the develop branch, but use JamesHeinrich\GetID3\GetID3; needs to be changed to use getID3; in /var/www/ampache/src/Module/Util/VaInfo.php.

I can now add catalogs

wagnered commented 4 years ago

Actually 2 happens every time I log out. After log in, I have to access index.php to get the landing page.

usox commented 4 years ago

Hi @wagnered

@usox

Uncaught Error: Call to a member function format() on string in /var/www/ampache/src/Config/Init/InitializationHandlerGlobals.php:43

Yep, this is a known one (have a look at the first post in the thread) but I wasn't able to spot the reason for it yet. I didn't not spend much time in search though.

added alias to use Ampache\Module\System\Core as SysCore; and used SysCore to create the class object.

1. Was then able to install.  After install and update, went to login page.  Upon logging in, got blank page.  I then set URL to fetch localhost:4000/index.php and went to Ampache's landing page.  This is repeatable,  if browser cache is deleted each time, otherwise might not be noticed.  I left to it to you to solve.

No problem. I'll go the extra mile tomorrow and will try to install the software freshly a few times to find issues within the installer.

2. This, for some reason didn't show up on the develop branch, but `use JamesHeinrich\GetID3\GetID3;` needs to be changed to `use getID3;` in `/var/www/ampache/src/Module/Util/VaInfo.php`.

Thanks, this one slipped through while merging. I fixed it now with bedb3d14c38275357869a5161d9ed9867e08c606

I can now add catalogs

I've the source-changes branch up an running for some weeks now - so it basically works but I'm sure, there will some more bugs come up :bug:

Again, thanks for testing.

wagnered commented 4 years ago

Track number isn't displayed in song edit dialog.

Haven't gained enough understanding of the view factory to discover problem. Will be necessary to step through with vdebug to gain better understanding of the problem.

wagnered commented 3 years ago

Don't know if it's significant but I'll report it anyway. Adding spotify credentials to the config files, I made a typo. This resulted in the URL: http://127.0.0.1:4000/test.php?action=config with a blank page.

usox commented 3 years ago

Track number isn't displayed in song edit dialog.

Haven't gained enough understanding of the view factory to discover problem. Will be necessary to step through with vdebug to gain better understanding of the problem.

The view wasn't working at all - it just displayed some random data (for me). Fixed with c7572a21105d8c84c0904ce760391d931f367df5

usox commented 3 years ago

Don't know if it's significant but I'll report it anyway. Adding spotify credentials to the config files, I made a typo. This resulted in the URL: http://127.0.0.1:4000/test.php?action=config with a blank page.

It's not directly related to your typo, it was a path issue :)

wagnered commented 3 years ago

Couldn't update file tags.

function getCatalogIds() in ".../Admin/Catalog/AbstractCatalogAction.php" always returns empty array.

Note that $catalog->isReady() is hard-coded true.

@lachlan-00: seems to me that isReady should be settable, especially when processing/creating large catalogs.

wagnered commented 3 years ago

Or maybe better yet, $catalog->show_ready_process(); and $catalog->perform_ready(); could be replaced with a queueing system that could provide job queueing for catalog work, mailing, and other tasks.

usox commented 3 years ago

Couldn't update file tags.

function getCatalogIds() in ".../Admin/Catalog/AbstractCatalogAction.php" always returns empty array.

Sic, thanks. Fixed it with 8b3e5f7186cdc886d9c5ded05ca5f3ecc64f6386.

usox commented 3 years ago

@wagnered btw

Haven't gained enough understanding of the view factory to discover problem. Will be necessary to step through with vdebug to gain better understanding of the problem.

If I could help you to get a better understanding of how the gui requests getting handled, let me know.

wagnered commented 3 years ago

Stepping through has provided me an understanding of concerning the various sctions, although the namespaces are little help to me in finding the file paths.

Could you clarify the rationale for using a "view factory" and the workflow for creating a view. I am somewhat familiar with the Laravel view engine and factory and yours seems much more complex.

There are a few more problems with finding and displaying the art to select. I'll comment on them in a while.

wagnered commented 3 years ago

Got the find/display artist art working:

  1. Added $keywords = $item->get_keywords(); to top of "public/templates/show_get_art.inc.php"

  2. Added use getID3; to top of "src/Model/Art.php"

Now on to finding "get album art" problems.

wagnered commented 3 years ago

Modifed art.php:378 to $type = 'Ampache\\Model\\' . ucfirst($this->type); to fix writing file tag.

Now on to finding "get album art" problems.

wagnered commented 3 years ago

Added $getType = 'getAlbum'; at line 100 in ".../Art/Collector/SpotifyCollectorModule.php" to fix find album problem.

usox commented 3 years ago

Got the find/display artist art working:

  1. Added $keywords = $item->get_keywords(); to top of "public/templates/show_get_art.inc.php"
  2. Added use getID3; to top of "src/Model/Art.php" Modifed art.php:378 to $type = 'Ampache\\Model\\' . ucfirst($this->type); to fix writing file tag. Added $getType = 'getAlbum'; at line 100 in ".../Art/Collector/SpotifyCollectorModule.php" to fix find album problem.

Thanks for having a look. At least some of this errors originate from merging the id3tag revert from develop - was a complicated merge.

usox commented 3 years ago

Could you clarify the rationale for using a "view factory" and the workflow for creating a view. I am somewhat familiar with the Laravel view engine and factory and yours seems much more complex.

You're talking about the GuiFactory in the Ampache\Gui namespace?

Well, the concept of this is not as complex as it may look like. Every template file (e.g. resources/templates/song.xhtml) has it's own adapter class (e.g. Ampache\Gui\Song\SongViewAdapter). The goal of this is to separate the model from the templates and to prevent the templates from accumulating logic - the adapter acts like an interface between the template and the model/application layer. This approach enables us to easily write unittests for most of the template related logic which would otherwise be nearly impossible if the logic resides in the templates.

The factory is used to simply create those adapters and helper classes like the TalView (which puts the template/template engine and the adapter together and renders the template). So an action (like the ShowSongAction in this example) just needs the GuiFactory to create the necessary classes to render the song detail view. This means, we have a small set of dependencies and will achieve 100% unittest coverage from request parsing til rendering.

But to be clear: This is still a PoC for me which can change any time or - if the team is not ok with it - can be reverted to the old system. Maybe the factory will get split up achieve a more domain-like model (e.g. a factory for every domain like Song) or something else.

wagnered commented 3 years ago

Thanks for the explanation.

Latest upstream fetch:

Failed opening required /src/Module/Util/../../../public/templates/show_song_row.inc.php'

It's not in the folder and not in git history.

As a mental exercise, I'll copy "show_song_row.inc.php" into the folder and try to adapt it to the new architecture.

Or should I just hold off on upstream fetching until my fixes are finished?

lachlan-00 commented 3 years ago

DB updates good!

will check the config update manually on the server to test

localplay menu gets a lot of errors

[Mon Nov 23 16:45:21.222195 2020] [proxy_fcgi:error] [pid 1652668:tid 139862013769472] [client 192.168.1.9:57875] AH01071: Got error 'PHP message: PHP Fatal error:  Uncaught ArgumentCountError: Too few arguments to function Ampache\\Module\\Application\\LocalPlay\\AbstractLocalPlayAction::__construct(), 1 passed in /var/www/ampache/src/Module/Application/LocalPlay/ShowPlaylistAction.php on line 50 and exactly 2 expected in /var/www/ampache/src/Module/Application/LocalPlay/AbstractLocalPlayAction.php:43\nStack trace:\n#0 /var/www/ampache/src/Module/Application/LocalPlay/ShowPlaylistAction.php(50): Ampache\\Module\\Application\\LocalPlay\\AbstractLocalPlayAction->__construct()\n#1 /var/www/ampache/vendor/php-di/php-di/src/Definition/Resolver/ObjectCreator.php(143): Ampache\\Module\\Application\\LocalPlay\\ShowPlaylistAction->__construct()\n#2 /var/www/ampache/vendor/php-di/php-di/src/Definition/Resolver/ObjectCreator.php(71): DI\\Definition\\Resolver\\ObjectCreator->createInstance()\n#3 /var/www/ampache/vendor/php-di/php-di/src/Definition/Resolver/ResolverDispatcher.php(71): DI\\Definition\\Resolver\\ObjectCreator->resolve()\n#4 /var/www/ampache/vendor/p...', referer: https://ampache.lachlandewaard.org/index.php

Browse of artist and album is okay except songs are missing from albums and song browses image

image

usox commented 3 years ago

@lachlan-00 Oh. First, I didn't understand what @wagnered said earlier regarding the song_row but now, I see. This worked until I added the Gatekeeper to the SongViewAdapter a few days ago. Sadly, the software suppresses errors (param is missing in a method call), so this kind of errors doesn't show up (it not in the log either). Will fix this later.

@wagnered The song_row is alreadly using the new system, so the template file is gone :innocent:

wagnered commented 3 years ago

had to revert to b3e0109299042171044b5e7f8597100ce3f59 to continue debugs/fixes. As I'm working on fixes I found problems with the way I configured writing images to tags.

Anyway, I'll hold off on fetching upstream while working on this.

usox commented 3 years ago

So, the song row view is back again with a78f745673e1298f1600fe16e2dc4f2e94871194

usox commented 3 years ago

DB updates good!

:balloon:

localplay menu gets a lot of errors

Uhh yes, parent constructor parameter missing. Didn't show up as an error, just a warning :disappointed: Fixed in 1de2069b130a41d6819a4a09acd71b21c436a346

wagnered commented 3 years ago

/var/www/ampache/src/Module/Api/Edit/RefreshUpdatedAction.php on line 62

It's requiring "song_row". Not causing a problem. My server is php -S localhost:4000 so I see all requests and most php errors in the cli.

wagnered commented 3 years ago

It's requiring "show_song_row.inc.php". The (subtle) problem is that after clicking "save" the edited song doesn't show up on the browse songs page until the page is refreshed.

lachlan-00 commented 3 years ago

will check the config update manually on the server to test

tested writing a config+1 update without issue

localplay menu gets a lot of errors

probably more missing from localplay, the show_playlist action seems to silently fail

2020-11-25T08:43:53+10:00 [user] (Ampache\Module\Application\ApplicationRunner) -> Found handler `Ampache\Module\Application\LocalPlay\ShowPlaylistAction` for action `show_playlist`
2020-11-25T08:43:53+10:00 [user] (mpd.class) -> Connecting to: 192.168.1.21:6600
2020-11-25T08:43:53+10:00 [user] (session.class) -> Session created: b30cbfcf15b80efb72b4a58688b9bf72

separately, the global for user is a user object, might be set as a string somewhere. image

[Wed Nov 25 08:43:15.123897 2020] [proxy_fcgi:error] [pid 1699282:tid 139789271758592] [client 192.168.1.13:58160] AH01071: Got error 'PHP message: PHP Fatal error:  Uncaught Error: Call to a member function format() on string in /var/www/ampache/src/Config/Init/InitializationHandlerGlobals.php:43\nStack trace:\n#0 /var/www/ampache/src/Config/Init/Init.php(55): Ampache\\Config\\Init\\InitializationHandlerGlobals->init()\n#1 /var/www/ampache/src/Config/Init.php(37): Ampache\\Config\\Init\\Init->init()\n#2 /var/www/ampache/public/index.php(32): require('/var/www/ampach...')\n#3 {main}\n  thrown in /var/www/ampache/src/Config/Init/InitializationHandlerGlobals.php on line 43'
lachlan-00 commented 3 years ago

stats.php getting time without an object type

2020-11-25T08:52:31+10:00 [user] (dba.class) -> Error_query SQL: SELECT `time` FROM `` WHERE `id` = ?
2020-11-25T08:52:31+10:00 [user] (dba.class) -> Error_query MSG: ["42000",1103,"Incorrect table name ''"]

image

wagnered commented 3 years ago

I was going to use core::get_request but the annotation indicated " * @deprecated Use RequestParser". So after finally locating the class/ interface I added use Ampache\Module\Util\RequestParserInterface;. But php wouldn't recognize the function getFromRequest. What am I not doing correctly?

lachlan-00 commented 3 years ago

okay, there are two things i want to do:

After those 2 items i'm going to put an ampache5 preview/pre-release on the release page as well as docker. After a preview period i should be able to document how to upgrade and we'll look at merge into develop early new year.

real excited to get this going, master is stable enough that i'm happy to not rely on keeping develop as the old branch once we can write down how to migrate things. (which i'm thinking the moodle way of just make a new folder and update your webserver)

wagnered commented 3 years ago

An observation: I was stepping through update.php and discovered that if $retval &= Dba::write($sql); is successful, the object "stmt" is returned thus an unreported error "Object of class PDOStatement could not be converted to int" thus $retval remains unchanged. Probably been like this since Ampache's birth.

usox commented 3 years ago

Unfortunately I have a lot of work at the moment (pre-Christmas madness at work), so I have very little free time at the moment. I'll take care of Composer 2 compatibility and the questions that came up here next. I'll get back to you later.