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

Issue 360: PHP 7.2 compatibility #541

Merged
merged 5 commits into from
Apr 22, 2019

Conversation

veggiematts
Copy link
Contributor

@veggiematts veggiematts commented Feb 20, 2019

PR for Issue #360, using provided shell script.

@veggiematts
Copy link
Contributor Author

Not tested yet, and I think statements like this : require_once "common/Object.php"; should also have been modified.

@veggiematts
Copy link
Contributor Author

Tested quickly with the follow-up, seems ok. Help wanted for some more tests.

@jeffnm
Copy link
Member

jeffnm commented Feb 21, 2019

@veggiematts I messed up somehow. I was pulling your biblibre:Issue_360_php_compat branch into my jeffnm:travis_ci_testing branch, but it looks like my commits from there have all ended up in on your branch too. I don't really understand how that happened.

I didn't think I had permissions to commit or merge to the Biblibre repository, but that seems to be what has happened, and I don't see anyway to reset it back to the previous state. The only explanation I can think of is that there was a merge conflict (I had renamed my Auth module's Object class already on that branch), and somehow in the process of resolving that it must have pushed my changes back to your repo. I wonder if it's because this PR was opened to CORAL-ERM's repo and I have permission through the CORAL-ERM organization to push changes back to the source branch of open pull requests?

On the plus side, the travis-ci integration is working well enough that it ran a passing build check on this pull request now that my changes for that are included? Sorry!

Let me know if I can help clean things up somehow.

@veggiematts
Copy link
Contributor Author

That's... strange. Anyway, I just pushed-force on the branch to revert it back to its previous state.

@jeffnm
Copy link
Member

jeffnm commented Feb 22, 2019

Thanks!

@Alleboom
Copy link

Hey Veggiematts, I made the same changes on our internal environement independently (before finding this issue) and it seems to be working ok. I'll let you know if I run into any issues.

@veggiematts
Copy link
Contributor Author

Thanks for the feedback @Alleboom.
@jeffnm , @t4k , are we merging this ?

@t4k
Copy link
Contributor

t4k commented Apr 18, 2019

It looks like a relatively simple change, and @jeffnm says it passed some rudimentary tests, so I would say go for it. It'll be easier to debug if need be when I am more actively working with the development branch locally.

@jeffnm jeffnm merged commit 56d1b4c into coral-erm:development Apr 22, 2019
@remocrevo remocrevo mentioned this pull request Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants