Allow generated classes target dir to be null

[matthiasnoback]

Because of the string casting, in my code I have to call setGeneratedClassesTargetDir() like this:

class HydrateUsingGeneratedHydrator implements Hydrate
{
    /**
     * @var string|null
     */
    private $cacheDir;

    public function __construct($cacheDir)
    {
        $this->cacheDir = $cacheDir;
    }

    public function hydrate(array $data, $object)
    {
        $configuration = new Configuration(get_class($object));

        if ($this->cacheDir !== null) {
            $configuration->setGeneratedClassesTargetDir($this->cacheDir);
        }
        ...
    }
}

The proposed change allows me to just call $configuration->setGeneratedClassesTargetDir($this->cacheDir); without the extra if clause.

[Ocramius]

This breaks internal data integrity of the Configuration object, and may cause exceptions at a later execution point in time.

[Ocramius]

@matthiasnoback I don't understand the use-case. What are you passing in as a path?

[matthiasnoback]

I agree that it isn't ideal from the point of view of internal data integrity. But null is a valid option here (since you later turn null into sys_get_temp_dir()). So I'd like to just blindly pass in either null or a string. So maybe it would be better to do something like:

if (!(is_string($generatedClassesTargetDir) || $generatedClassesTargetDir === null)) {
    // throw exception
}

$this->generatedClassesTargetDir = $generatedClassesTargetDir;

[matthiasnoback]

Maybe I wasn't clear: the problem is that my null value becomes '' because of (string).

[Ocramius]

@matthiasnoback should I apply stricter validation and throw an exception? I'm fine with that approach.

[matthiasnoback]

That, or maybe

$this->generatedClassesTargetDir = $generatedClassesTargetDir === null ? null : (string) $generatedClassesTargetDir;

[Ocramius]

@matthiasnoback I'd rather always expect a string, and use the sys_get_tmp_dir() only as a fallback for when nothing was set explicitly (at all)

[matthiasnoback]

Okay, no problem. Then I guess we'll leave it as is.

[Ocramius]

@matthiasnoback the docblock says string, so if you want to add an exception, feel free to do so.

[matthiasnoback]

Well, in theory the current code would also accept __toString()-able objects, so let's not change that. This was just a really small inconvenience to begin with. Closing this now :) Thanks for your time.