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

Jump to bottom

Removes some more unneeded code #15395

Merged
merged 1 commit into from Jan 16, 2020
Merged

Removes some more unneeded code #15395

merged 1 commit into from Jan 16, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jan 15, 2020

No description provided.

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jan 15, 2020
@sgiehl sgiehl added this to the 4.0.0 milestone Jan 15, 2020
tsteur
tsteur reviewed Jan 15, 2020
View reviewed changes
misc/composer/build-xhprof.sh
@@ -2,7 +2,7 @@

if ! type phpize &> /dev/null; then
echo "phpize missing, skipping build"
echo "If you installed PHP via Aptitude, you can install phpize w/ 'sudo apt-get install php5-dev'"
echo "If you installed PHP via Aptitude, you can install phpize w/ 'sudo apt-get install php7-dev'"
Copy link
Member

Choose a reason for hiding this comment

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

does xhprof even still work with PHP7? AFAIK not?

Copy link
Member

Choose a reason for hiding this comment

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

I think we could just remove the script... anyone can install it manually anyway if needed

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to work with php7 meanwhile: http://pecl.php.net/package/xhprof

Copy link
Member Author

Choose a reason for hiding this comment

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

shall I remove the whole package from composer then, or only the scripts to be executed?

Copy link
Member

Choose a reason for hiding this comment

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

Sweet, looks like it was made compatible just last month maybe. We can also keep it then as it is. No Preference. Feel free to merge the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Have no preference either. But could you have a look at the composer script. Guess that needs a change. But I'm not using xhprof at all, so it's hard for me to check what needs to be done there:

matomo/core/Composer/ScriptHandler.php

Lines 30 to 33 in b16a791

if (!self::isPhp7orLater()) {
// doesn't work with PHP 7 at the moment
passthru('misc/composer/clean-xhprof.sh');
}

Copy link
Member

Choose a reason for hiding this comment

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

Removed the isPhp7orLater for a test and it at least didn't fail for me on PHP 7.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.

ok. removed the checks

@tsteur tsteur merged commit 1361185 into 4.x-dev Jan 16, 2020
@tsteur tsteur deleted the removedeadcode branch January 16, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur left review comments

Assignees
No one assigned
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
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sgiehl @tsteur