Skip to content

[Cache] Log a more readable message when trying to cache an unsupported type #31395

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

Merged
merged 1 commit into from
May 11, 2019

Conversation

Deuchnord
Copy link

@Deuchnord Deuchnord commented May 6, 2019

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

Improved the warning risen when trying to save something that has a non-supported type in the Simple cache.

For instance, let's say the following code:

class TestCommand extends Command
{
    private $cache;

    public function __construct(CacheInterface $cache)
    {
        parent::__construct();
        $this->cache = $cache;
    }

    // ...

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $n = $this->cache->get('n', function () {
            return function () {
                return rand(0,10);
            };
        });

        dump($n());
    }
}

Running this code will give the following:

14:32:03 WARNING   [cache] Could not save key "n" in cache: the Closure type is not supported.
0

@stof
Copy link
Member

stof commented May 6, 2019

this is not logging the exception anymore.

@Deuchnord Deuchnord force-pushed the cache-improve-type-warning branch from f3e14ab to ab1eb51 Compare May 6, 2019 14:29
@Deuchnord
Copy link
Author

Travis failure is not related.

@Deuchnord Deuchnord force-pushed the cache-improve-type-warning branch from 5d1ff9d to ae099b7 Compare May 8, 2019 07:31
@Deuchnord
Copy link
Author

Deuchnord commented May 8, 2019

Tests failures on AppVeyor are not related: they seem to be related to changes made on other components, whereas I only reworded warning-level log messages on the Cache component.

@Deuchnord Deuchnord changed the title Log a more readable message when trying to cache an unsupported type [Cache] Log a more readable message when trying to cache an unsupported type May 8, 2019
@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 8, 2019

I'm sorry but I don't see why this improves the state of things.
The message is essentially the same, with a hint ("incompatible type") that might be wrong - as this place doesn't know why the failure happened (could be the type, but something else)
Also, logging the $id instead of the $key doesn't provide useful info (the $id is internal)

The brackets are not explicit enough thought.
I'd suggest something like this instead:
'Failed to save key "{key}" of type %s: '.$e->getMessage() (last part removed when there is no $e)

@Deuchnord
Copy link
Author

Thanks @nicolas-grekas, sorry, looks like I didn't understand the original issue like this, I'll fix my PR ASAP :)

@nicolas-grekas nicolas-grekas added this to the next milestone May 9, 2019
@Deuchnord Deuchnord force-pushed the cache-improve-type-warning branch 2 times, most recently from 4bd2d3e to ec281fe Compare May 9, 2019 18:52
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 May 11, 2019
@nicolas-grekas nicolas-grekas changed the base branch from master to 4.3 May 11, 2019 10:08
@nicolas-grekas nicolas-grekas force-pushed the cache-improve-type-warning branch from ec281fe to 21ba3c0 Compare May 11, 2019 10:15
@nicolas-grekas
Copy link
Member

Thank you @Deuchnord.

@nicolas-grekas nicolas-grekas merged commit 21ba3c0 into symfony:4.3 May 11, 2019
nicolas-grekas added a commit that referenced this pull request May 11, 2019
… an unsupported type (Deuchnord)

This PR was merged into the 4.3 branch.

Discussion
----------

[Cache] Log a more readable message when trying to cache an unsupported type

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29710   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        |

Improved the warning risen when trying to save something that has a non-supported type in the Simple cache.

For instance, let's say the following code:
```php
class TestCommand extends Command
{
    private $cache;

    public function __construct(CacheInterface $cache)
    {
        parent::__construct();
        $this->cache = $cache;
    }

    // ...

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $n = $this->cache->get('n', function () {
            return function () {
                return rand(0,10);
            };
        });

        dump($n());
    }
}
```

Running this code will give the following:
```
14:32:03 WARNING   [cache] Could not save key "n" in cache: the Closure type is not supported.
0
```

Commits
-------

21ba3c0 [Cache] Log a more readable error message when saving into cache fails
nicolas-grekas added a commit that referenced this pull request May 23, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

[Cache] improve logged messages

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

This was improved already in #31395, but the patch was incomplete.
This PR fixes this.

Commits
-------

257f3f1 [Cache] improve logged messages
nicolas-grekas added a commit that referenced this pull request Jun 5, 2019
This PR was squashed before being merged into the 4.3 branch (closes #31864).

Discussion
----------

[Cache] Fixed undefined variable in ArrayTrait

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

So once again (see #29591) my test suite managed to find an error in ArrayTrait in cache. This time it was this PR: #31395 later improved by #31590 that introduced `$id` to logging, which I guess should be `$key`? So this PR changes it to `$key`, ~but my tests **still fail** as there is no `$this->namespace` in `ArrayAdapter` (is this the only class that uses this ArrayTrait?). But I don't know what to do about it. Maybe @nicolas-grekas has some answers?~

Commits
-------

8568923 [Cache] Fixed undefined variable in ArrayTrait
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.

5 participants