Skip to content

SERIOUS BUG IN NEW VERSION #557

Closed

Description

@GrahamCampbell

Requests are being cached incorrectly, so that API calls to get a users repo are now returning cached results from the wrong user. I am rolling back to the previous version immediately.

Activity

  1. acrobat commented on Mar 31, 2017

    @acrobat
    Collaborator

    I think the generated cache key is to general. See https://github.com/php-http/cache-plugin/blob/master/src/CachePlugin.php#L290

    If the request parameters are included the key will be unique enough I think

    Thoughts on this @Nyholm?

  2. GrahamCampbell commented on Mar 31, 2017

    @GrahamCampbell
    ContributorAuthor

    This is only in the very latest update. No such bug occurred 7 days ago.

  3. acrobat commented on Mar 31, 2017

    @acrobat
    Collaborator

    That's because the caching of requests was broken in previous versions. All requests with the private or no-cache directive were skipped by the cache plugin. So no github api request could be cached

    You can fix this for now by passing this option to the cache plugin. Example from the docs:

    use Cache\Adapter\Redis\RedisCachePool;
    
    $client = new \Redis();
    $client->connect('127.0.0.1', 6379);
    // Create a PSR6 cache pool
    $pool = new RedisCachePool($client);
    
    $client = new \Github\Client();
    $option = [ 'respect_response_cache_directives' => ['no-cache', 'private', 'max-age', 'no-store']];
    $client->addCache($pool, $options);
  4. acrobat commented on Mar 31, 2017

    @acrobat
    Collaborator

    @GrahamCampbell Can you provide some example call which fails when using the cache? So I can check how to fix this

  5. Nyholm commented on Mar 31, 2017

    @Nyholm
    Collaborator

    Thank you for reporting this. I will make a PR.

  6. Nyholm commented on Apr 5, 2017

    @Nyholm
    Collaborator

    I've added #558. Please review it.

    We are just waiting for a new tag on the CachePlugin.

  7. GrahamCampbell commented on Apr 5, 2017

    @GrahamCampbell
    ContributorAuthor

    Also, the current version of this package is throwing a deprecation error, even know I'm not using a deprecated feature?

  8. Nyholm commented on Apr 5, 2017

    @Nyholm
    Collaborator

    Also, the current version of this package is throwing a deprecation error, even know I'm not using a deprecated feature?

    From where?

  9. GrahamCampbell commented on Apr 5, 2017

    @GrahamCampbell
    ContributorAuthor
  10. GrahamCampbell commented on Apr 5, 2017

    @GrahamCampbell
    ContributorAuthor

    Surely this still won't fix everything. Shouldn't the caching have only been etag based, why on earth would we provide a correct etag, and then fetch the wrong cache response?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions