duck7000 / imdbGraphQLPHP

5 stars 0 forks source link

Is providing own custom cache interface in Title() supported? #58

Closed mosource21 closed 1 month ago

mosource21 commented 1 month ago

I am trying to migrate from tboothman/imdbphp and I use the following and provide my own cache interface.

$imdb = new \Imdb\Title($id, $myconfig, NULL, $mycache);

For whatever reason my cache is never used - there are no calls to the set / get methods.

If I change back to using the tboothman bootstrap.php it works change to the imdbphp6 bootstrap.php it doesn't.

If it should work are there any tips on how I can try to debug?

Thanks

duck7000 commented 1 month ago

First of all cache wasn't even supported at all, one of the users here asked for it and i added back the default cache system used in imdbphp. i don't even no how a custom cache would work to be honest. For my purpose of imdbphp6 cache is not needed so i removed it at first. For the record I'm not a professional programmer, i don't have any programming education at all, only self thought. And this project is not a 100% clone or fork of imdbphp, i used it as base so yes it looks the same but not fully.

When i started this project i had 2 goals: Strip anything out that i not absolutely needed. (the kiss principle) The second goal was to provide support that actually works (i left imdbphp out of frustration of the lack of support and hostility against PR). That is why i started out with a minimal version (granted that is grown to almost a full blown version by now)

So i can deliver support for al that is working at this point, but not for enhancements which i thought of nobody will use (i don't use (custom) config or cache but my other users do use default cache

So to answer your question (after a long story) No custom cache is not supported, you may find left overs that indicate otherwise but i never payed any attention to that. (i should probably remove Config $config = null, CacheInterface $cache = null from __construct in title.php?)

Use your own config is also not supported

mosource21 commented 1 month ago

Problem The constructors in Title.php and MdbBase.php no longer matched causing any specified custom cache to be silently ignored and falling back to defaults (normally no cache) due to $cache actually being sent to the $logger parent constructor

Solution Title.php (see below - tested working for me but this is just a quick fix for my specific needs so I am not sure if this impacts other things) Name.php (may need a similar adjustment but untested)

From:

    /**
     * @param string $id IMDb ID. e.g. 285331 for https://www.imdb.com/title/tt0285331/
     * @param Config $config OPTIONAL override default config
     * @param CacheInterface $cache OPTIONAL override the default cache with any PSR-16 cache.
     */
    public function __construct($id, Config $config = null, CacheInterface $cache = null)
    {
        parent::__construct($config, $cache);
        $this->setid($id);
    }

To:

    /**
     * @param string $id IMDb ID. e.g. 285331 for https://www.imdb.com/title/tt0285331/
     * @param Config $config OPTIONAL override default config
     * @param LoggerInterface $logger OPTIONAL override default logger `\Imdb\Logger` with a custom one     
     * @param CacheInterface $cache OPTIONAL override the default cache with any PSR-16 cache.
     */
    public function __construct($id, Config $config = null, LoggerInterface $logger = null, CacheInterface $cache = null)
    {
        parent::__construct($config, $logger, $cache);
        $this->setid($id);
    }
duck7000 commented 1 month ago

Thanks for your comments!

But as i understand it right the only problem been the number of parameters not correct according to your call? I don't see what adding logger to __construct() would do or help? Logger is not used anywhere except in API.php so no point to add it to title or name class.

$imdb = new \Imdb\Title($id, $myconfig, NULL, $mycache); // has 4 parameters

So what about this: $imdb = new \Imdb\Title($id, $myconfig, $mycache); // it matches the parameters from the construct

I'm not sure custom cache would even work though

mosource21 commented 1 month ago

Thanks for replying.

I wish to apologise if I am not explaining very well or not understanding correctly myself.

Custom cache can work - it is for me with this minor change.

$imdb = new \Imdb\Title($id, $myconfig, $mycache); // it matches the parameters from the construct

Yes this would make the most sense but remember when this is called you run the __construct of the Title.php class

//this is in Title.php
public function __construct($id, Config $config = null, CacheInterface $cache = null)
{     
    parent::__construct($config, $cache);
    $this->setid($id);
}

So the line parent::construct($config, $cache) will then run the construct of the MdbBase.php class

//this is in MdbBase.php
public function __construct(Config $config = null, LoggerInterface $logger = null, CacheInterface $cache = null)
{
    $this->config = $config ?: $this;
    $this->logger = empty($logger) ? new Logger($this->debug) : $logger;
    $this->cache = empty($cache) ? new Cache($this->config, $this->logger) : $cache;
    $this->graphql = new GraphQL($this->cache, $this->logger, $this->config);
}

So as the code stands at the moment in the Title.php construct the $cache variable is actually being sent in to the MdbBase.php construct variable $logger placeholder

        parent::__construct(       $config       ,                 $cache                                      );   //this is in Title.php
public function __construct(Config $config = null, LoggerInterface $logger = null, CacheInterface $cache = null);   //this is in MdbBase.php 

Apply my fix

//apply the fix in Title.php
public function __construct($id, Config $config = null, LoggerInterface $logger = null, CacheInterface $cache = null)
{
    parent::__construct($config, $logger, $cache);
    $this->setid($id);
}

So I added a NULL to my $imdb line to make the parameters match up

        parent::__construct(       $config       ,                           NULL,                $cache       );   //this is in Title.php
public function __construct(Config $config = null, LoggerInterface $logger = null, CacheInterface $cache = null);   //this is in MdbBase.php 

Other ways to fix

  • Add $logger back to the __construct in Title.php
  • or Remove $logger throughout as currently you remove from Title.php class and find it is needed by other classes like GraphQL.php so it stops things from working.
duck7000 commented 1 month ago

mm i am getting what you are referring to, the parent construct parameters don't match up, i never noticed that before!

So the simplest solution is to add logger back to title construct like you suggested, although it will never be used inside title class but won't hurt either.

I'll will fix this, thanks for pointing this out.

duck7000 commented 1 month ago

Fixed in latest commit.

@mosource21 will you check if this is working as it should?

mosource21 commented 1 month ago

I have seen this (and your other commits) but I am calling it a day now will report back tomorrow when I will be refreshed and can give it my full attention. Thanks

mosource21 commented 1 month ago

All commits looking good, custom cache now working fine and no problems with "random" upper case characters. Thanks 👍

duck7000 commented 1 month ago

I'm glad we worked out the remaining/last bugs!

Feel free to report other issues, it will benefit us all