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

Fix / Cleanup Tracker\TableLogAction::isActionTypeStoredUnsanitized() #12314

Closed
sgiehl opened this issue Nov 30, 2017 · 3 comments
Closed

Fix / Cleanup Tracker\TableLogAction::isActionTypeStoredUnsanitized() #12314

sgiehl opened this issue Nov 30, 2017 · 3 comments
Assignees
@diosmosis
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone
3.6.0

Comments

@sgiehl
Copy link
Member

sgiehl commented Nov 30, 2017

While trying to resolve another issue, I spotted this piece of code.
Not sure if it is a bug or if it just can be removed, as the code can't be working since 2014:

    /**
     * @param $actionType
     * @return bool
     */
    private static function isActionTypeStoredUnsanitized($actionType)
    {
        $actionsTypesStoredUnsanitized = array(
            $actionType == Action::TYPE_DOWNLOAD,
            $actionType == Action::TYPE_OUTLINK,
            $actionType == Action::TYPE_PAGE_URL,
            $actionType == Action::TYPE_CONTENT,
        );

        return in_array($actionType, $actionsTypesStoredUnsanitized);
    }

I don't think this method can return anything else than false. It's only used once, so maybe we can simply remove it.
@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. labels Nov 30, 2017
@sgiehl sgiehl added this to the 3.2.2 milestone Dec 4, 2017
@mattab
Copy link
Member

mattab commented Dec 14, 2017 •
edited

@sgiehl It looks like this method is very buggy indeed.
However if I read this correctly, it would still return true if and only $actionType is Action::TYPE_PAGE_URL ? ($actionType == Action::TYPE_PAGE_URL would be true, and in_array(1, array(true)) would return true.

@sgiehl
Copy link
Member Author

sgiehl commented Dec 14, 2017

seems we are both wrong. just tested that on command line. The method will always return true unless the value of $actionType is false (casted to boolean)

@diosmosis diosmosis self-assigned this Jun 20, 2018
@diosmosis
Copy link
Member

Started looking into this and thought it was a bug, but turns out it actually works as expected (in a very strange way). Here are two scenarios:

  1. when $actionType == Action::TYPE_DOWNLOAD:

$actionsTypesStoredUnsanitized is set to:

array(
            true,
            false,
            false,
            false,
)

in_array(true-ish, $actionsTypesStoredUnsanitized); resolves to true as expected.

  1. when $actionType == Action::TYPE_EVENT_NAME:

$actionsTypesStoredUnsanitized is set to:

array(
            false,
            false,
            false,
            false,
)

in_array(true-ish, $actionsTypesStoredUnsanitized); resolves to false as expected.

It's certainly weird code, but not broken. Will create a PR to make it less confusing.

@diosmosis diosmosis mentioned this issue Jun 21, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@diosmosis diosmosis

Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Milestone
3.6.0
Development

No branches or pull requests
3 participants
@diosmosis @mattab @sgiehl