-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
Use zf1s packages to gain php 7.2 compatibility #670
Conversation
This comment has been minimized.
This comment has been minimized.
9e0facf
to
93d0120
Compare
This comment has been minimized.
This comment has been minimized.
e14452e
to
5425adf
Compare
modern distro support is going to be huge. 👍 👍 👍
does a list of use cases to test exist anywhere? |
I don't believe there is a list of test cases. In theory we would have tests written with Selenium that would automatically test various functionalities but the existing selenium scripts don't appear to be working. Other than just trying to click around and see if everything works I don't think anyone has developed a specific plan. I'll try to play around with it and maybe I can play around with selenium testing but my last attempt didn't make it very far. |
Do we have a dev setup guide lying around somewhere? Preferably for someone who has never used Vagrant before... |
No guide in particular but |
Thanks. I'll try that and add some docs to the wiki |
So I just tried this via vagrant up ubuntu-bionic and I got the Rest error with z1fs I think that is related to the change you merged with zf1s but for some reason it isn't applied upon installation. I'll look in further. Using PHP 7.2.10-0ubuntu0.18.04.1 (cli) (built: Sep 13 2018 13:45:02) ( NTS ) So it looks like line in composer.json Update: Not sure why it is requiring this step. |
OK, I'll rerun the composer update and repush the results... I should find some time tomorrow, for then please run it to test, that reflects what we should be comitting anyway. |
Yeah so far the UI seems pretty functional after the update. I've been able to add podcasts and create smartblocks. |
@paddatrapper There are some docs on the vagrant setup at http://libretime.org/vagrant/ maybe we should merge those with your efforts. |
@hairmare I will create a MR with any changes I have to that instruction list then |
Looking at your wiki page adding something about the |
I just pushed the |
i just checked out
|
update: i got past this one with
had some files in there that were owned by root. not sure why. now continuing troubleshoot:
|
LibreTime should be up and running now on localhost:8080. I also can't get the docs build to work on anything, but it isn't required to test this |
oh, sure enough!!
thanks! checking use cases now |
works great!!! i checked: track import, playlist creation/editing, webstream creation, podcast creation/edit, calendar manipulation, master source connection, stream settigns changes, user creation/edit, user assign to show, login/logout, user settings edit, set autoloading playlist. i obviously haven't clicked every single thing, but the only functionality areas i haven't tested are those that are scheduled, like podcast download, and autoloading playlists. |
update: an autoloading playlist i scheduled worked, as did automatic download of a podcast. i think we're ready to go! |
What do we want to do before we push this ? |
I don't think there is anything else required to merge this. Are there any doc related things that need updating to reflect new php support? The support matrix in the wiki at least will need changes to Debian Buster and Ubuntu Bionic |
I'd like to do the 3.0.0-alpha.7 release pretty soon after we merge this as it will expose it to a larger audience. I think the milestone covers what's still open for that to happen. When updating in-situ with a vagrant install from master you might need to remove the vendor for everything to work out. Releases shouldn't be affected by this issue. If we want to I can update this to use
as the dirty hack for ensuring a proper locale on Ubuntu Bionic. I'm assuming we would want to push some fixes for the locale issue before we announce official Bionic and Buster support. Maybe we can figure out how to get rid of the hack completely. Checking Buster so see if it's also affected by the PostgreSQL issue/hack could also be of interest. |
I'm happy to test the Buster postgreSQL locale hack on Monday. The rest of the milestone is pretty much dependant on this PR though. I can't really test the install without the new zf1s dependencies fixing the Vagrant build |
It seems that Buster install works fine without the hack, but locales are generated in the installer later on here Running this on Buster using PHP 7.3, I get the following error:
|
PHP has this in their docs
So it looks like we'll need to fix some more stuff before we're ready for 7.3... Most likely initializing I'll look into taking care of this later this week. |
I rebased this onto master, removed the locale hack and, force pushed it. My rationale for removing the locale hack is that we need to look into it and it shouldn't be introduced as part of this pr. This only supports up to php 7.2 for now but I think we should merge it soonish rather than wait for php 7.3 support. 7.3 is in the pipeline and I'll make more update PRs when the needed fixes are here. Let me know if I should update anything else so we can get this merged. |
Sounds good to me. I'm willing to go ahead with the merge. |
i already tested functionality following this change as systematically as i reasonably could and it worked great. i won't be able to test the most recent vagrant changes until later this week, and know Robbt is deep in another problem today. @paddatrapper to the rescue? happy rainy sunday folks, take care |
It started up fine with vagrant so I'm going to merge it. |
This PR replaces the zend/zendframework1 dependency with composer packages from the zf1s orga. The zf1s packages already contain loads of fixes to allow zend framework 1 to run on php 7.2 and the maintainer is willing to merge more PHP 7.2 compat changes as needed. The switch to modularized zf1 packages will also make replacing zf1 piece by piece easier if we decide to go down that route.
This will allow us to run LibreTime on Ubuntu Bionic as well as the upcoming CentOS 8 which both bump PHP to 7.2.
As mentioned in #580 (comment) I've tested this on Ubuntu 18.04 (Bionic Beaver) as well as Ubuntu 14.04 (Trusty Thar). I checked that the program grid, uploading tracks, basic playout and, podcast downloading works.
This is a rather large change and as such I'd like to ask everyone to do some serious testing with this change to ensure that all or your use cases still work. If you find any issues, please post the output of
php --version
as well as the full error message and stacktrace from either Apaches error.log or the zendphp.log and I'll fix them in this PR. To be sure we don't break any existing use cases there also needs to be some testing on the old still supported PHP 5.x versions.Fixes #580