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

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

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

netniV
Copy link
Contributor

@netniV netniV commented Jun 29, 2018

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

Closes #144

@peter279k
Copy link

peter279k commented Jul 23, 2018

@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
Copy link
Contributor Author

netniV commented Jul 23, 2018

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
Copy link

@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
Copy link
Contributor Author

netniV commented Jul 23, 2018

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
Copy link

@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
Copy link
Owner

howardjones commented Jul 23, 2018 via email

@howardjones
Copy link
Owner

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.

@howardjones howardjones merged commit ce885bf into howardjones:master Jul 23, 2018
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.

None yet

3 participants