Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@Deuchnord
Copy link

@DeuchnordDeuchnord commentedMay 6, 2019
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#29710
LicenseMIT
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 TestCommandextends Command{private$cache;publicfunction__construct(CacheInterface$cache)    {parent::__construct();$this->cache =$cache;    }// ...protectedfunctionexecute(InputInterface$input,OutputInterface$output)    {$n =$this->cache->get('n',function () {returnfunction () {returnrand(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 commentedMay 6, 2019

this is not logging the exception anymore.

Deuchnord reacted with thumbs up emoji

@DeuchnordDeuchnordforce-pushed thecache-improve-type-warning branch fromf3e14ab toab1eb51CompareMay 6, 2019 14:29
@Deuchnord
Copy link
Author

Travis failure is not related.

@DeuchnordDeuchnordforce-pushed thecache-improve-type-warning branch from5d1ff9d toae099b7CompareMay 8, 2019 07:31
@Deuchnord
Copy link
Author

Deuchnord commentedMay 8, 2019
edited
Loading

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.

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

nicolas-grekas commentedMay 8, 2019
edited
Loading

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 reacted with thumbs up emoji

@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-grekasnicolas-grekas added this to thenext milestoneMay 9, 2019
@DeuchnordDeuchnordforce-pushed thecache-improve-type-warning branch 2 times, most recently from4bd2d3e toec281feCompareMay 9, 2019 18:52
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3May 11, 2019
@nicolas-grekasnicolas-grekas changed the base branch frommaster to4.3May 11, 2019 10:08
@nicolas-grekasnicolas-grekasforce-pushed thecache-improve-type-warning branch fromec281fe to21ba3c0CompareMay 11, 2019 10:15
@nicolas-grekas
Copy link
Member

Thank you@Deuchnord.

Deuchnord reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commit21ba3c0 intosymfony:4.3May 11, 2019
nicolas-grekas added a commit that referenced this pull requestMay 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     <!-- seehttps://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:```phpclass 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 requestMay 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 requestJun 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

5 participants

@Deuchnord@stof@nicolas-grekas@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp