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

Make Matomo PHP 7.3 compatible #13418

Closed
5 tasks done
Findus23 opened this issue Sep 12, 2018 · 13 comments
Closed
5 tasks done

Make Matomo PHP 7.3 compatible #13418

Findus23 opened this issue Sep 12, 2018 · 13 comments
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@Findus23
Copy link
Member

Findus23 commented Sep 12, 2018

In 3 months PHP 7.3 will be released, so let's start making Matomo compatible!

I tried setting up a new Matomo instance using PHP 7.3 RC2 (so all bugs found now will probably also be in the final release) and most things are working fine. These errors occurred:


ErrorException: strpos(): Non-string needles will be interpreted as strings in the future. Use an explicit chr() call to preserve the current behavior
#17 vendor/tedivm/jshrink/src/JShrink/Minifier.php(196): handleError

Stack Trace
from https://github.com/tedious/JShrink

  • fixed

2018/09/12 20:46:17 [error] 8919#8919: *137700 FastCGI sent in stderr: 
"PHP message: PHP Warning:  
"continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"?
in /var/www/matomo-beta/vendor/tedivm/jshrink/src/JShrink/Minifier.php on line 243"

also from https://github.com/tedious/JShrink

  • fixed

WARNING [2018-09-12 19:04:26] 20837  /var/www/matomo-beta/plugins/LanguagesManager/Commands/CreatePull.php(217): Warning - "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"? - Matomo 3.6.0 - Please report this message in the Matomo forums: https://forum.matomo.org (please do a search first as it might have been reported already)

switch ($returnCode) {
case 401:
$output->writeln("Pull request failed. Bad credentials... Please try again");
continue;


ErrorException: compact(): Undefined variable: idGoal
#15 plugins/API/API.php(401): handleError

Stack Trace


ErrorException: Undefined index: never
#15 core/View/HtmlReportEmailHeaderView.php(41): handleError
#14 core/View/HtmlReportEmailHeaderView.php(41): __construct
#13 core/ReportRenderer/Html.php(84): renderFrontPage

Stack Trace


To be continued...

If you want to, I can create test-admin accounts on the php 7.3 instance.

@fdellwing
Copy link
Contributor

fdellwing commented Sep 12, 2018

During visitorgenerator:generate-visits. I am not sure what exactly is the error and if it has anything to do with PHP 7.3.

Not a PHP error, it tries to write -5 into an db field that is unsigned mostlikely. Why it tries to write -5 is a interesting question nethertheless.

Also note that your request contains 2 token_auths, that is quite unusual I would say..

@Findus23 Findus23 added the Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Oct 21, 2018
@Findus23
Copy link
Member Author

Okay, I have now updated the server to the latest release candidate and tried around and couldn't really find more issues.

For the first two I'll create a pull request soon once tedious/JShrink#80 is fixed.

@fdellwing I agree that this is probably a really obscure visitor generator bug, so I have removed it here.

Interestingly everything around PHP generation still works fine (where I expected most bugs).

As PHP7.3 just migrated to Debian testing, I'll expect it to be the default version in the next Debian stable (will be released in about a year) and ubuntu versions. Therefore probably a lot of people will be using it in a year.

@Findus23
Copy link
Member Author

Okay, I mixed a bit up regarding JShirk. We are using a more than 4 year old beta version of it.
The latest version from last month fixes the warnings in PHP 7.3, so it should be easy to update, but unfortunately the recent versions dropped support for 5.5 which causes composer to refuse installing it.

Does anyone have an idea what's the easiest way to fix this?

@sgiehl
Copy link
Member

sgiehl commented Oct 21, 2018

If the current version is actually still compatible with 5.5, but the dependencies were just changed in the composer.json we could overwrite it with a custom package in composer.json to overwrite the php requirement. e.g.:

{
    "type": "package",
    "package": {
        "name": "tedious/jshrink",
        "description": "Javascript Minifier built in PHP",
        "keywords": ["minifier","javascript"],
        "homepage": "http://github.com/tedious/JShrink",
        "type": "library",
        "license": "BSD-3-Clause",
        "version": "v1.3.1",
        "authors": [
            {
                "name": "Robert Hafner",
                "email": "tedivm@tedivm.com"
            }
        ],
        "require": {
            "php": ">=5.5.0"
        },
        "autoload": {
            "psr-0": {"JShrink": "src/"}
        },
        "source": {
            "type": "git",
            "url": "https://github.com/tedious/JShrink",
            "reference": "v1.3.1"
        }
    }
}

This was referenced Oct 22, 2018
@Findus23
Copy link
Member Author

@sgiehl Thanks for the tip.
I had to rename the package so that composer would use it instead of the real one.

@sgiehl
Copy link
Member

sgiehl commented Nov 8, 2018

Got another one:

"continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"?
/home/travis/build/piwik/vendor/tecnickcom/tcpdf/tcpdf.php:17778

Need to check if that will be fixed with #13566

@J0WI
Copy link

J0WI commented Dec 11, 2018

#11936 is still open. Shouldn't this be resolved first?

@J0WI
Copy link

J0WI commented Dec 11, 2018

Please also clarify this in the docs. Currently it just says "PHP version 5.5.9 or greater".

@sebash1992
Copy link

@sgiehl could you do any hot fix for the tcpdf issue?

@sgiehl
Copy link
Member

sgiehl commented Jan 16, 2019

@sebash1992 this should be fixed in the upcoming 3.8.0

@socialmecpl
Copy link

socialmecpl commented Jan 22, 2019

Replace continue with break resolved the issue.

@mattab mattab added this to the 3.8.1 milestone Jan 22, 2019
@mattab mattab modified the milestones: 3.8.1, 3.9.0 Jan 28, 2019
@diosmosis
Copy link
Member

@Findus23 since JShrink was updated, is this issue done?

@Findus23
Copy link
Member Author

Findus23 commented Jan 29, 2019

@diosmosis If no one else reports any more issues, I'd say this is done.
Next week I'll move my live instance to 7.3 and can probably then report back more issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

8 participants