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

Profiler dot client #299

Merged
merged 2 commits into from Jan 3, 2019
Merged

Profiler dot client #299

merged 2 commits into from Jan 3, 2019

Conversation

sowbiba
Copy link
Contributor

@sowbiba sowbiba commented Jan 2, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets fixes #298
License MIT

What's in this PR?

Slugify client name used as selector to remove non standard characters
https://www.w3.org/TR/CSS21/syndata.html#characters

Why?

#298

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

cool, thanks! i think the approach with a twig function is good enough, but we need to avoid name collisions.

Collector/Twig/SlugifyExtension.php Outdated Show resolved Hide resolved
Collector/Twig/SlugifyExtension.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

looks like a good fix to me.

Update Changelog

Rename extension from slugify to httplug_slugify

Rename extension from slugify to httplug_slugify

Rename extension from slugify to httplug_slugify
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. I like the way you addressed this problem. A TwigExtension do make sense if there are a lot of values to slugify. However, It might be a bit overkill in this scenario.

Why couldn't modify Collector::getClientRootStacks to return ['slug' => $client]?

Then we could add a new variable here name clientSlug or something. That would be a 10 line fix, right?

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

that is even simpler, i like it!

@sowbiba
Copy link
Contributor Author

sowbiba commented Jan 3, 2019

@Nyholm Thanks for the suggestions. Changes made

@Nyholm
Copy link
Member

Nyholm commented Jan 3, 2019

Excellent. Thank you @sowbiba

@dbu dbu merged commit f94de3b into php-http:master Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Profiler does not show stack when client name contains dots
3 participants