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

Fixes lot of things #15648

Closed
wants to merge 14 commits into from
Closed

Fixes lot of things #15648

wants to merge 14 commits into from

Conversation

tolbon
Copy link
Contributor

@tolbon tolbon commented Mar 2, 2020

Convert certain Tabs to Spaces,
Fix PHP Docs
Type declaration in function
Fix potential php errors

@Findus23 Findus23 added the Needs Review PRs that need a code review label Mar 2, 2020
sgiehl
sgiehl requested changes Mar 2, 2020
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left a couple of comments. Not sure if any type hint might cause any problems, but we'll see if the tests are still passing...

core/Period.php Outdated Show resolved Hide resolved
libs/upgradephp/upgrade.php Outdated Show resolved Hide resolved
libs/upgradephp/upgrade.php Outdated Show resolved Hide resolved
plugins/CoreHome/DataTableRowAction/RowEvolution.php Show resolved Hide resolved
plugins/CoreVisualizations/Visualizations/Sparklines.php Outdated Show resolved Hide resolved
plugins/Installation/FormFirstWebsiteSetup.php Show resolved Hide resolved
@tolbon
Copy link
Contributor Author

tolbon commented Mar 3, 2020

I see lot of failing compilation introduce by my modification I fix this asap

@tolbon
Copy link
Contributor Author

tolbon commented Mar 3, 2020 •
edited

I need help a test don't pass but I don't see where I need to revert
https://travis-ci.org/matomo-org/matomo/jobs/657749446#L877
https://travis-ci.org/matomo-org/matomo/jobs/657749446#L925

I didn't find where this xml is generated

Edit : It seems good but row goal is before row action and expected is action before goal
I don't know why :-/

@sgiehl
Copy link
Member

sgiehl commented Mar 3, 2020

you can ignore the both tests mentioned. They are failing on 4.x-dev as well. Guess that's a sorting issue between PHP 7.2 and PHP 7.3 or something like that.

@tolbon
Copy link
Contributor Author

tolbon commented Mar 3, 2020

I know it’s too late but ...
My commit is very big, Very hard for you to reviews this no ? Can I do something
Do you want I close this PR and do more little PR ?

@sgiehl
Copy link
Member

sgiehl commented Mar 4, 2020

Having smaller PRs makes it a lot easier to review and check why tests are failing. This PR has still a lot failing tests, but it's hard to say which change made them fail. So might be better to split the changes

@tolbon
Copy link
Contributor Author

tolbon commented Mar 4, 2020

Yeah I close this and make little Pull Request, sorry for this

@tolbon tolbon closed this Mar 4, 2020
@tolbon tolbon deleted the Fixes branch March 31, 2020 19:44
@mattab mattab added this to the 4.0.0 milestone Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl requested changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tolbon @sgiehl @mattab @Findus23