search

howardjones / network-weathermap

Network Weathermap draws diagrams from data
http://www.network-weathermap.com/
MIT License
426 stars 95 forks source link

Fix string assert() calls which breaks PHP 7.2 #145

Closed netniV closed 6 years ago

netniV commented 6 years ago

This PR corrects issues with the assert() function requiring the passed assertion to be a boolean.

Closes #144

peter279k commented 6 years ago

@netniV, It seems that the Travis CI build has been stopped. After checking the Travis build log, the sh ./dev/Vagrant/vagrant-cacti-develop.sh has some problems. The settings.sh is not existed and I also found that it only has the settings.sh-sample in dev/Vagrant.

In the vagrant-cacti-develop.sh file, it should be modified the setting.sh file path on line 13, too.

I think it should do cp ./dev/Vagrant/settings.sh-sample ./dev/Vagrant/settings.sh before executing sh ./dev/Vagrant/vagrant-cacti-develop.sh command.

I also found that it should run the composer install in install block and add the php-7.2 test in Travis CI build so that it can check whether the php-7.2 should be passed by phpunit test.

netniV commented 6 years ago

Those are issues with @howardjones's build scripts. You should log them as a separate issue and point them at the travis build from this PR to demonstrate it. I have never even got running with a vagrant system yet.

peter279k commented 6 years ago

@netniV, you're right. But the Travis CI build script should be fixed and this PR can check whether the php-7.2 will be passed by phpunit test in Travis CI build.

netniV commented 6 years ago

This is @howardjones project and i'm just a contributor as I work on the cacti project this integrates with.

I won't personally be touching those as they are part of the repo's structure / design (imho) that @howardjones has settled upon and should be handled by him (unless he really wants me to).

This PR was purely for the assert problem. I am sure he will appreciate what you've identified. You could even submit a PR with the changes you recommend.

peter279k commented 6 years ago

@netniV, I know that. And I just notice that issue because the Travis CI build should be worked. And it can let every oncoming PR be verified. I know this issue I mention is not related to this PR, but I hope you can know what I concern.

Thanks.

howardjones commented 6 years ago

@peter279k Thanks for some insight on the TravisCI script! I actually enabled both TravisCI and CircleCI at different times, but haven't had the patience to make the dozens of trial & error commits so that the build script actually builds :-)

The actual phpunit scripts do work though - I use those offline all the time. Unfortunately, they partly use image comparisons to work, and the output of GD is subtly different between different PHP versions.

On 23 July 2018 at 17:56, Mark Brugnoli-Vinten notifications@github.com wrote:

This is @howardjones https://github.com/howardjones project and i'm just a contributor as I work on the cacti project this integrates with.

I won't personally be touching those as they are part of the repo's structure / design (imho) that @howardjones https://github.com/howardjones has settled upon and should be handled by him (unless he really wants me to).

This PR was purely for the assert problem. I am sure he will appreciate what you've identified.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/howardjones/network-weathermap/pull/145#issuecomment-407127187, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3sJZLzs-ijdMEdl-u7doHSBckhbjGbks5uJgAvgaJpZM4U8mFH .

howardjones commented 6 years ago

Travis actually fails on legit test failures now (including highlighting the issue in this PR, for PHP 7.2, which is nice!). The image comparison issue I mentioned means that a bunch of tests still fail though. I'll need to generate references images for each version before that can be fixed.