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

setter functions changed as fluent. #19

Merged
merged 1 commit into from
Apr 12, 2016
Merged

setter functions changed as fluent. #19

merged 1 commit into from
Apr 12, 2016

Conversation

shibby
Copy link
Contributor

@shibby shibby commented Apr 6, 2016

i've changed all setters functions as fluent.
because of they are not fluent, it was really hard setting parameters.

$piwik = new PiwikTracker($id,$url);
$piwik->setSomething(1);
$piwik->setSomething(2);
$piwik->setSomething(3);

now we can use all setter functions like:

$piwik = new PiwikTracker($id,$url);
$piwik->setSomething(1)
         ->setSomething(2)
         ->setSomething(3);

i think thats more good way for setting sth.

Also, i've made change on setForceVisitDateTime function.
Modern php is really pretty, let use basic objects :) Now this functions accepts DateTimeInterface, so we can pass DateTime object

*/
public function setForceVisitDateTime($dateTime)
{
$this->forcedDatetime = $dateTime;
if ($dateTime instanceof DateTimeInterface) {
Copy link
Member

Choose a reason for hiding this comment

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

As DateTimeInterface was introduced in PHP 5.5 I presume this would fail on PHP 5.3 etc so we can't really merge this change. You could maybe check if the interface exists then it could be ok. Any other thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, DateTimeInterface comes with 5.5.0. i didn't think backward compability. We can use DateTime instead of interface. What do you think? Maybe we can remove whole DateTime object passing if you think its not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe remove it for now and when working on a new version of PHP Tracker we could use DateTimeInterface by default.

@tsteur
Copy link
Member

tsteur commented Apr 6, 2016

Fluent is okay I would say. When we refactor it and have a pure data object for config etc we might make the setter methods immutable but in this case it should be fine when it's mutable.

Any other thoughts on this?

@tsteur tsteur added this to the Current sprint milestone Apr 6, 2016
@shibby
Copy link
Contributor Author

shibby commented Apr 8, 2016

I've removed commits about DateTime objects.

@tsteur
Copy link
Member

tsteur commented Apr 12, 2016

Awesome 👍 Thanks for this

@tsteur tsteur merged commit 8a48a22 into matomo-org:master Apr 12, 2016
@innocraft-automation innocraft-automation removed this from the Current sprint milestone Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants