I think if we can avoid having this logic in two places, it would be better. From looking at the Controller::setGeneralVariablesView() method, it looks like the reason it's difficult to pull this out, is because each variable (or nearly each) is set as a property on the view.

Maybe we can create a getCurrentPeriodInfo() method (w/ or w/o a better name) that returns an array with all the information needed in setGeneralVariablesView(). In setGeneralVariablesView(), you'd set the result as properties on $view, and in CoreHome:: getDateString() you'd just use the localized long string. Can you do something like that here?

Might look something like:

class Controller
{
    protected function getCurrentPeriodInfo()
    {
        $dateStart = // ...
        $dateEnd = // ...
        // ...
        $prettyDateLong = // ...

        return [
            'dateStart' => $dateStart,
            'dateEnd' => $dateEnd,
            // ...
            'prettyDateLong' => $prettyDateLong,
        ];
    }

    protected function setGeneralVariablesView($view)
    {
        // ...

        $dateInfo = $this->getCurrentPeriodInfo();
        foreach ($dateInfo as $name => $value) {
            $view->$name = $value;
        }

        // ...
    }
}

Sounds good to me 👍 I used the name getCurrentDatePeriodVars() (can change if advised).

I struggled a bit to extract the side-effects that are tangled up with this "date and period" logic: specifically, the calls to Period::checkDateFormat($rawDate) which throws if date is invalid, and $this->setDate($validDate), which modifies a property on $this.

I decided to leave these side-effects in the getCurrentDatePeriodVars() method. For the former, this validation seems important. For the latter, there would be a fair amount of logic left duplicated. Let me know if you have any input on this!

I see what you mean about setDate(), those side effects could result in confusion in the future. setDate() sets the $this->strDate & $this->date properties on the controller, but looks like they aren't used in getCurrentDatePeriodVars(), so we could potentially move the setDate() call to after $dateVars = $this->getCurrentDatePeriodVars();.

Something like:

// in getCurrentDatePeriodVars() return the $validDate variable too...
$validDate = null;
if ($periodStr != 'range') {
    // ...

    $validDate = ...
} else {
    // ...
}

// ...

return [
    // ...
    'validDate' => $validDate,
];

then in setGeneralVariablesView():

$dateVars = $this->getCurrentDatePeriodVars();
if (Date::factory($this->strDate)->toString() != $validDate->toString()) {
    // ... comment ...
    $this->setDate($validDate);
}

then the side effect will be limited to the setGeneralVariablesView() method. Think it'll work?

Yep, I think that will work just fine!

The getLongPrettyDate() method doesn't take parameters, so no need to pass these two in.

🤦‍♂️

I'm guessing you're using the $injector here to avoid a dependency cycle? Would be nice if we could avoid the cycle, but I can't think of a simple way to do it...

Correct. I couldn't see a simple solution around it either :/

Don't need piwikApi here, correct?

Hm, yes I think you are correct. I don't remember why I added this 🤷‍♂️

This would need to be an API method, and would also need to check for permissions.

Like Piwik::checkUserHasSomeViewAccess();

Hi @tsteur, good catch on the missing function-level access control! I can add this permission check.

What does it mean, this needs to be an API method? Does it need to get documented somewhere, or some code changed?

It would be documented kind of automatically once it is an API method. As suggested below, I would maybe remove all changes in PHP and instead only add the date in JS to the title (always). This would remove a lot of problems and complexity plus ensure consistent behaviour.

I wonder if we could keep it simple and if the date was changed in JS, simply show the same date format in the title as the date picker does? It wouldn't be hugely formatted, but maybe good enough.

It would simplify a few things in this PR for sure. Controller::getLongPrettyDate() would not be needed (I think), so all changes to core/plugin/Controller.php and plugins/CoreHome/Controller.php could be reverted.

A non-essential XHR would also be avoided (tiny perf gain for the front-end app)

@spacenate Sounds good, would be great if you can make these changes 👍

We cannot hardcode "Piwik" here (which should be "Matomo" btw), because when for example a custom logo is used, we do not show "Matomo" in the title.

I don't know how this can be solved as it'll be hard to detect which part of the title needs to be replaced with a title. And the title may be different to the title specified here.

I wouldn't even mind if we just removed the date from the title in general.

Thinking about it... I reckon if we still wanted to have the date in the page title, I would say lets only set the date in JS and not in PHP/Twig at all. This will be easiest and allows us to change the title etc. and have logic only in one place. So JS could append the date to whatever title is currently set, and when date is changed, remove the last part and update it again. Would make it a lot simpler and more consistent.

I really like the idea of de-duplicating this logic. It would be simple to cache the title that was generated on the back-end, so no tomfoolery is needed when updating the title again (just append the new date string to the cached title). Shall I make this change in this PR?

This PR or another PR 👍

prettyDateLong does not seem to be used in any other view templates. Should I remove it from core/Plugin/Controller.php:L(633)?

Other plugins might make use of it, so better to keep it IMO. Though you could leave a note in Controller.php that it's not used by core.

👍

Other plugins might make use of it, so better to keep it IMO. Though you could leave a note in Controller.php that it's not used by core.

Noticed this isn't really necessary, could probably remove it. Below since $location.search() is called, updatePeriodParamsFromUrl() ends up being called in piwikService. It would be useful outside of an angular context, but that only happens in the embedded pages, and since those are shown in iframes, the document title isn't really that useful

Ok! I wasn't certain about if or when angular might not be rendering the page, so I added this here to be safe. It sounds like it is unnecessary, so I will go ahead and remove it.