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

Added custom method to filter urls #24

Merged
merged 4 commits into from Mar 27, 2019

Conversation

Projects
None yet
3 participants
@Mte90
Copy link
Contributor

commented Mar 26, 2019

This patch eanble with a custom function in config.php to clean up the url from parameters:

    'profiler.clean_url' => function($uri) {
        $uri = str_replace('?enable-tideways', '', $uri);
        $uri = str_replace('%3Fenable-tideways', '', $uri);
        return $uri;
    }
Added custom method
This patch eanble with a custom function to clean up the url from parameters:

```if (!function_exists('custom_validate_url')) {
    function custom_validate_url($uri) {
        $uri = str_replace('?enable-tideways', '', $uri);
        $uri = str_replace('%3Fenable-tideways', '', $uri);
        return $uri;
    }
}```

@Mte90 Mte90 referenced this pull request Mar 26, 2019

Merged

Tideways with Xhgui #18

@Mte90 Mte90 changed the title Added custom method Added custom method to filter urls Mar 26, 2019

@glensc

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

you are modifying the $uri, how it's related to validating?

also, do not add a global symbol. add config key with closure as a value, like 'profiler.enable' already exists. pay attention to backward compatibility.

@Mte90

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Yes validating is not the better term, maybe is better like clean_url.
I am not sure how to pass a callback from the config and execute it inside the project and I saw that the function was the most easy way.
Do you have any hints for it?

@glensc

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I gave you the hint. search that string through this project.

@Mte90

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Looking at https://github.com/perftools/xhgui-collector/search?q=profiler.enable&unscoped_q=profiler.enable the project is using the callback without sending to them any parameters.
I already looked in the code but an example for my needs to check there wasn't, this was also one of the reason of a custom function.

@glensc

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

so, add the parameter you need...

@Mte90

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

rewritten as anonymous function in the config array, let me know if now is good :-)

@glensc

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

the change is ok, but it should be somehow documented, perhaps add a working example to default config (return $url itself)

but looking at current defaults, how is your filter different from 'profiler.simple_url':

@lauripiisang

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

I think the point here is to ensure that no sensitive data is saved and have control over how the replacements are done, e.g. ?api_token=1abc325&do_thing=thing1 -> ?api_token=XXXXXX&do_thing=thing1 .
simple_url adds a property, but $url still gets saved - https://github.com/perftools/xhgui-collector/pull/24/files#diff-3737a2b54402ad429752791444589b15R173

@lauripiisang

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

add documentation about this property into the default config, then it can be merged. I'd call the property replace_url or process_url, as it's really more abstract than clean, but I don't have that strong of an opinion.

@Mte90

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

I preferred a new function because on https://github.com/perftools/xhgui readme is explained that simple_url is used to remap better the urls or remove the pagination https://github.com/perftools/xhgui/blob/master/config/config.default.php#L55

@lauripiisang lauripiisang merged commit 0d0e010 into perftools:master Mar 27, 2019

1 check passed

Scrutinizer No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.