Navigation Menu

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

Clean up arobases #6185

Closed
wants to merge 5 commits into from
Closed

Clean up arobases #6185

wants to merge 5 commits into from

Conversation

kylekatarnls
Copy link
Contributor

Delete arobases and use Common::sendHeader when possible.
Issue: #6184

Please re-test it, I have some environment bugs.

@kylekatarnls
Clean up arobases
24a5630 
Delete arobases and use Common::sendHeader when possible.
@mattab
Copy link
Member

mattab commented Sep 10, 2014

Thanks it looks really good!

Can you please rebase as I can't merge it anymore unfortunately. Looking forward to seeing more of your pull requests!

(and could you maybe confirm whether you have tested it?) cheers

@mattab mattab added the Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Sep 10, 2014
@mattab mattab added this to the Piwik 2.7.0 milestone Sep 10, 2014
@kylekatarnls
Copy link
Contributor Author

I found out some others arobases. Many in core/Common.php, and before ini_set. And a @Header() remain in BaseFactory. So if I found some time, I will create a new one like this.

@mattab
Copy link
Member

mattab commented Sep 14, 2014

@kylekatarnls would you mind rebasing so we can check whether the tests still pass after your change? I'd be happy to merge it then 👍

@kylekatarnls
Copy link
Contributor Author

Yes, I planned to redo it from a clean new branch today (french hour).

@mattab mattab modified the milestones: Piwik 2.8.0, Piwik 2.7.0 Sep 14, 2014
@kylekatarnls
Copy link
Contributor Author

This one can be merged: #6211
I will complete it in a second issue if tests pass.
Oups, it was green a minute ago.
Maybe some tests must be updated?
I do not understand, Travis says "error in core/Filesystem.php line 75 : undefined variable: ata" but at this line, there is a comment.
I'm running the tests localy but it's not finished yet.

@kylekatarnls
Copy link
Contributor Author

I try to rebase it again.

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@kylekatarnls @mattab