Skip to content

Updated the "PHP config" panel in the profiler #20697

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

Closed
wants to merge 4 commits into from

Conversation

javiereguiluz
Copy link
Member

@javiereguiluz javiereguiluz commented Nov 30, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

I propose to update this panel taking some of the ideas introduced by @ro0NL in #20126:

  • Adding more info that helps debugging problems (like 32/64 bits, the locale and the timezone)
  • Removing anything related to PHP acceleration that is not OPcache or APC

Before

php-config-before

After

php-config-after

'xdebug_enabled' => extension_loaded('xdebug'),
'eaccel_enabled' => extension_loaded('eaccelerator') && ini_get('eaccelerator.enable'),
'apc_enabled' => extension_loaded('apc') && ini_get('apc.enabled'),
Copy link
Member

Choose a reason for hiding this comment

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

We could even drop the case of APC here, because APC does not have its opcode caching features anymore on PHP 5.5+ (as it can only be installed thanks to the BC layer of APCu)

Copy link
Member Author

Choose a reason for hiding this comment

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

In 5a7f20a I've changed this to only check for APCu. Is that a better idea or should we remove this APC check entirely? Thanks!

Copy link
Contributor

@robfrawley robfrawley Dec 3, 2016

Choose a reason for hiding this comment

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

@javiereguiluz I'd prefer to retain APCu check (but not APC-original, as removed in 5a7f20a)

@@ -67,11 +67,11 @@ public function collect(Request $request, Response $response, \Exception $except
'env' => isset($this->kernel) ? $this->kernel->getEnvironment() : 'n/a',
'debug' => isset($this->kernel) ? $this->kernel->isDebug() : 'n/a',
'php_version' => PHP_VERSION,
'php_architecture' => PHP_INT_SIZE * 8,
'php_locale' => \Locale::getDefault() ?: 'n/a',
Copy link
Member

Choose a reason for hiding this comment

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

technically, this is the Intl locale, not the PHP locale (which would be the one set by setlocale

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noticing this. What should we do here?

  1. Relabel this to Intl locale
  2. Get the PHP locale in a different way
  3. Remove this for not being reliable

Copy link
Member

Choose a reason for hiding this comment

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

I would relabel it. The Intl locale is more useful than the PHP locale in Symfony, as our own code does not deal with the PHP locale (it would be useful to display it if you were using ext-gettext to handle translations)

@nicolas-grekas
Copy link
Member

Just throwing the idea even if that's not for this PR: would be great to have a panel/section to display APCu & opcache stats, based on apcu_cache/sma_info() & opcache_get_status().

@javiereguiluz
Copy link
Member Author

@nicolas-grekas that a good idea! What if we add those to the upcoming "Cache" panel?

@javiereguiluz javiereguiluz added this to the 3.3 milestone Nov 30, 2016
@ro0NL
Copy link
Contributor

ro0NL commented Nov 30, 2016

@javiereguiluz do you plan to extract x.y.z from the full PHP version?

ie. mine is 5.6.23-1+deb.sury.org~trusty+2 for instance, not sure if that works out with this design.

@javiereguiluz
Copy link
Member Author

@ro0NL in #20126 you posted an screenshot and indeed the PHP version looked awful. What about demoting the long version suffix?

php-version

We could do the same for unstable PHP versions:

php-version-unstable

@ro0NL
Copy link
Contributor

ro0NL commented Nov 30, 2016

👍

@fabpot
Copy link
Member

fabpot commented Dec 13, 2016

Thank you @javiereguiluz.

@fabpot fabpot closed this Dec 13, 2016
fabpot added a commit that referenced this pull request Dec 13, 2016
…eguiluz)

This PR was squashed before being merged into the 3.3-dev branch (closes #20697).

Discussion
----------

Updated the "PHP config" panel in the profiler

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

I propose to update this panel taking some of the ideas introduced by @ro0NL in #20126:

* Adding more info that helps debugging problems (like 32/64 bits, the locale and the timezone)
* Removing anything related to PHP acceleration that is not OPcache or APC

### Before

![php-config-before](https://cloud.githubusercontent.com/assets/73419/20751739/b557ca9a-b6fd-11e6-98c4-49e80b16d424.png)

### After

![php-config-after](https://cloud.githubusercontent.com/assets/73419/20751740/b7da5c38-b6fd-11e6-8619-3d3b5f477887.png)

Commits
-------

531053b Updated the "PHP config" panel in the profiler
fabpot added a commit that referenced this pull request Dec 15, 2016
…gizanagi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[WebProfilerBundle] Fix PHP extensions in the toolbar

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes (adding other extensions info)
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20697
| License       | MIT
| Doc PR        | N/A

`ConfigDataCollector::hasAccelerator()` has been removed.

Except if we want to reintroduce the method, I'd suggest to add apcu & opcache infos directly (or just remove the accelerator line).

Commits
-------

2495030 [WebProfilerBundle] Fix PHP extensions in the toolbar
fabpot added a commit that referenced this pull request Dec 19, 2016
This PR was merged into the 3.3-dev branch.

Discussion
----------

[WebProfilerBundle] Split PHP version if needed

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes/no
| Fixed tickets | #20697 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

Commits
-------

1241a00 [WebProfilerBundle] Split PHP version if needed
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull request Dec 19, 2016
This PR was merged into the 3.3-dev branch.

Discussion
----------

[WebProfilerBundle] Split PHP version if needed

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes/no
| Fixed tickets | symfony/symfony#20697 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

Commits
-------

1241a00 [WebProfilerBundle] Split PHP version if needed
@nicolas-grekas nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017
@fabpot fabpot mentioned this pull request May 1, 2017
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
…(javiereguiluz)

This PR was squashed before being merged into the 3.3-dev branch (closes symfony#20697).

Discussion
----------

Updated the "PHP config" panel in the profiler

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

I propose to update this panel taking some of the ideas introduced by @ro0NL in symfony#20126:

* Adding more info that helps debugging problems (like 32/64 bits, the locale and the timezone)
* Removing anything related to PHP acceleration that is not OPcache or APC

### Before

![php-config-before](https://cloud.githubusercontent.com/assets/73419/20751739/b557ca9a-b6fd-11e6-98c4-49e80b16d424.png)

### After

![php-config-after](https://cloud.githubusercontent.com/assets/73419/20751740/b7da5c38-b6fd-11e6-8619-3d3b5f477887.png)

Commits
-------

531053b Updated the "PHP config" panel in the profiler
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.

7 participants