Bug: method monthNo() from class MDBbase should check if empty result

[jcvignoli]

Description

method monthNo() in MDBbase doesn't check if it gets an empty result before sending data to Person class. This results in a undefined index php notice if empty: int(8) string(22) "Undefined index: 2009," string(124) "Imdb/MdbBase.php" int(171) array(1) { ["mon"]=> string(5) "2009," }

Movies / TV-Shows / Person

Jorge Rivero, nm0729473, when using interviews() method

Comment

method monthNo() hides its failures behind a "@", this is a bad practice.

Proposal for fixing

I'm not an experienced developper at all and I lack of basic php knowldege, but I'd like to suggest a fix anyway:

    protected function monthNo($mon)
    {
    $return = '';
    if ( ! empty ($this->months[$mon]) ) {
            $return = $this->months[$mon];
        }
        return $return;
    }

[tboothman]

Presumably the real bug here is that it's trying to use 2009 as a month

[jcvignoli]

The bug? You mean, in my code?

[tboothman]

In your description you put "Undefined index: 2009" which is surely referring to trying to do $this->months[2009] which would mean something thought 2009 was a month when it used that method.

[jcvignoli]

Hi there,

I execute the method $this->person->pubmovies() where $this is the current class, and person the Imdb\Person class.

[duck7000]

Like @tboothman said the real problem here is that some interviews don't start with a date but with a year like jorge rivero but others too. (Jack Nicholson) The method used here is parsearticles() in person.php line 935. This line get wrong data (year instead of date) and parses it to monthNo, hence the error. I'm not really sure how to fix that, it would involve restructuring the preg_match from line 931 i guess? Or simply leave out the month number?

[duck7000]

I'm working on this to fix it, there are a lot of problems with parsearticles()

[duck7000]

@jcvignoli can you test my PR to ensure it works correctly? if so, @tboothman it would be nice if it get merged into master so it can benefit everyone?

[jcvignoli]

@duck7000 it works! :)

[duck7000]

Thanks for testing! Now we have to wait if and when it will be merged, but in the meantime you or anyone else at least can use it.

[duck7000]

Still waiting for this to be reviewed/merged

[duck7000]

Too bad for you i closed this PR, i have enough of the lax attitude here. I hope you did get the fixed version, if not we can arrange something.

[jcvignoli]

Hope it will be included in a next release!

[duck7000]

I doubt it..