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

started working on PHP 7 support #8706

Merged
merged 4 commits into from Sep 22, 2015
Merged

started working on PHP 7 support #8706

merged 4 commits into from Sep 22, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Sep 3, 2015

refs #8689

This is only partial PHP 7 support. More fixes will be needed.

There is a big blocker re usort. It will be hard to make system and UI tests working under PHP5.X and PHP 7.X see #8689 (comment) . Ideas?

I also created matomo-org/component-decompress#4

I believe once we fixed the sort problem and the decompress we will have it almost done

@tsteur tsteur added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 3, 2015
@@ -93,8 +103,8 @@ before_install:
install:
- git fetch -q

- export GENERATE_TRAVIS_YML_COMMAND="php ./tests/travis/generator/main.php generate:travis-yml --core --verbose"
- '[[ "$TRAVIS_JOB_NUMBER" != *.1 || "$TRAVIS_PULL_REQUEST" != "false" ]] || ./tests/travis/autoupdate_travis_yml.sh'
# - export GENERATE_TRAVIS_YML_COMMAND="php ./tests/travis/generator/main.php generate:travis-yml --core --verbose"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can enable it again but this means we need to update travis to maybe also run tests for plugins on PHP 7?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could regenerate the core .travis.yml w/ --php-versions=7,5.6,5.3.3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to merge matomo-org/travis-scripts#9 soon. That should be fine right? Or will it trigger to many auto updates etc? @diosmosis

@tsteur tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Sep 3, 2015
@sgiehl
Copy link
Member

sgiehl commented Sep 6, 2015

I've had a look at the changes... Did you try to (un)install / (de)activate plugins using php7? That doesn't work for me. I tried to figure out why, seems the config is not updated...

small note: when using php 5.5.20 with this branch handling the plugins works

@tsteur
Copy link
Member Author

tsteur commented Sep 7, 2015

I haven't tested that yet. Neither the installation (although we have tests for that). It's just a start so far, it will be much more work I think to make it actually compatible and will need much more testing. It's just a start to make the test run and to fix some things that were covered by tests

@mattab mattab modified the milestone: 2.15.1 Sep 8, 2015
@tsteur
Copy link
Member Author

tsteur commented Sep 8, 2015

I can confirm activate / deactivate doesn't work. I tried to debug but I couldn't use the interactive debugger so it was hard to debug. I really do not understand the code in IniFileChain class even though I tried for quite a while. The problem is dumpConfig() returns null. I think it's not a problem of that method itself but of some references and because of some changes before. The solution will be probably something like https://github.com/piwik/piwik/pull/8706/files#diff-359a72f49802c7978351b7d4c8c1fe8cR43 for config somewhere. Ideally we would rewrite Config and remove all those references etc and clean up that code of IniFailChain. I will mention this point in the original issue and we can fix it in a separate pull request. I think we should work on this thing first as maybe some tests fail because of that

@mattab mattab mentioned this pull request Sep 8, 2015
11 tasks
@tsteur
Copy link
Member Author

tsteur commented Sep 9, 2015

Activating / deactivating plugin should work now

@sgiehl
Copy link
Member

sgiehl commented Sep 9, 2015

I can confirm that (de)activating plugins works with the latest changes 👍

@mattab mattab added this to the 2.15.0 milestone Sep 11, 2015
@mattab
Copy link
Member

mattab commented Sep 11, 2015

Targeting 2.15.0 - if we don't merge it for 2.15.0 we should in a separate PR include the composer.lock update

@diosmosis
Copy link
Member

Started looking at this, but won't be able to finish a review for a day or two.

@mattab
Copy link
Member

mattab commented Sep 21, 2015

Fyi: the system test failure should be fixed after a rebase. LGTM

mattab pushed a commit that referenced this pull request Sep 22, 2015
started working on PHP 7 support
@mattab mattab merged commit 895df02 into master Sep 22, 2015
@diosmosis diosmosis deleted the 8689 branch September 22, 2015 10:20
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 13, 2015
@hpvd
Copy link

hpvd commented Oct 19, 2015

just a question on this:
This Issue is assigned to 2.15 milestone but it has the not in changelog lable.

=> will 2.15 work fine on php 7.0?

If yes, the speed gains should be promoted greatly (if there are any)

On other platforms, php 7.0 is a huge step:
http://www.zend.com/en/resources/php7_infographic

@tsteur
Copy link
Member Author

tsteur commented Oct 19, 2015

It's not fully compatible yet. I presume that's why it is not mentioned there. It might work fun but there could be some problems. I reckon there will be most likely a minor update for PHP 7 once it is released in a few weeks.

@hpvd
Copy link

hpvd commented Feb 3, 2016

this issue is marked as done in #8689

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants