Add a developer-friendly way to keep the file until the end of the request

[kohlerdominik]

This PR adds a simple method to keep a reference on the instance until the request ends.

When calling $file->keepDuringRequest(), a register_shutdown_function is registered, which simply holds the instance in a variable. As long as the callback is waiting for it's call, the instance is therefore referenced somewhere, meaning it's destructor is not called and the file therefore not yet deleted.

This is usefull, if a temporary file is used later in the request, but it's not possible to pass the instance. This happens most likely because at some point a string parameter is enforced (like in my case).

[mikehaertl]

@kohlerdominik While I see your point I'm a bit unsure if we really should include this. It somehow doesn't fit well with the existing interface and adds too much "magic" (e.g. makes it hard to debug). Currently we only delete the file in the destructor and you can suppress this by setting delete to false. But it should actually be three options:

1. Delete in destructor or
2. delete on shutdown or
3. don't delete at all

The implementation should reflect this (e.g. with a single property autoDelete that you can set to one of three constants). But this would be a BC break which I want to avoid.

For now I would prefer if we could only add an example to the README for how to delete the in a custom shutdown method:

$file = new File('bla bla', '.html');
$file->delete = false;
$filename = $file->getFileName();
register_shutdown_function(function () use ($filename) {
    @unlink($filename);
});

Could you live with that?

[kohlerdominik]

Hi @mikehaertl

I totally agree with you. If "delete" wasn't a property, but a setter method, it would be much better. However, we can't change that, so i did go for an extra method.

Another apporach would be adding methods over that and encurage new implementations to prefert the methods, marking the delete property as deprecated.

e.g.

public function dontDelete()
{
    $this->delete = false;
}

public function deleteOnShutdown()
{
    $this->dontDelete()
    register_shutdown_function(fn() => @unlink($this->getFileName()))
}

or (less prefered for me)

public function delete($when)
{
    switch($when) {
        case: 'never':
            $this->delete = false;
            break;
        case 'onShutdown':
            $this->delete = false;
            register_shutdown_function(fn() => @unlink($this->getFileName()));
            break;
      case 'onDestruct':
            $this->delete = true;
            break;
      default:
            throw new \InvalidArgumentException();
    }
}

Regarding the readme example: in my opinion, this is no real solution. When reading code, it might be not very obvious what it does, while calling a method with a speaking name describes this very good. So for writing the code, a readme example would help to resolve the issue, but for maintenance it's probably better to extend the class and add the method, so it describes the feature properly.

However, as maintainer the decision is up to you. For me it's ok to replace your library with my fork.

[mikehaertl]

I would prefer something like your second example because it's really a choice between 3 options. The interface should clearly reflect that. Having 2 methods here somehow obfuscates this. It's also not clear what happens if you call both methods (or if you need to do that) without looking at the implementation.

Anyhow it's something for a 2.0 release. Before I create that I wonder if there are more features missing. If you have ideas please let me know. (Apart from #16 where I don't have a satisfying solution yet)

[kohlerdominik]

I would prefer something like your second example because it's really a choice between 3 options. The interface should clearly reflect that. Having 2 methods here somehow obfuscates this. It's also not clear what happens if you call both methods (or if you need to do that) without looking at the implementation.

I can agree with that. But i'm not a big fan of text-based input for methods with only a few parameters. Maybe we can use constants like brick/math does?

Anyhow it's something for a 2.0 release. Before I create that I wonder if there are more features missing. If you have ideas please let me know. (Apart from #16 where I don't have a satisfying solution yet)

For me, there's not really missing functionality. But from Symfony and Laravel, I'm used to have some convinient helpers available on classes like this. To name a couple possibilities:

public function getContent() : string
{
    return file_get_contents($this->getFileName())
}

public function getResource(string $mode = "r+") : resource
{
    return fopen($this->getFileName(), $mode);
}

public function appendContent(string $content) : self
{
    file_put_contents($this->getFileName(), $content, FILE_APPEND)

    return $this;
}

[mikehaertl]

I can agree with that. But i'm not a big fan of text-based input for methods with only a few parameters. Maybe we can use constants like brick/math does?

Yeah, that was my idea, too. Something like:

public const DELETE_NEVER = 'never';
public const DELETE_ON_DESTRUCT = 'on_destruct';
public const DELETE_ON_SHUTDOWN = 'on_shutdown';
public function delete($type = self::DELETE_ON_DESTRUCT) { ... }

On the other hand, the default value doesn't make much sense - as this is the default anyway (you don't have to call delete() at all, to get this behavior). Hmm, let's see if we find something better.

Your other suggestions sound reasonable. I'm just not sure about the return type declarations. It would bump the PHP version constraint to some PHP 7. Right now we're still compatible with 5.4 - but maybe that's really a bit extreme. Not sure yet.

[kohlerdominik]

Hi Mike

It's been a while, and with every dependency upgrade I find my old post here, checking wheter I can remove my fork from the dependency list. Unfortunatelly, still stuck with it.

With php8.1 we finally have enums. It could make this a little cleaner, like:

enum DeleteType {
  case Never;
  case OnShutdown;
  case OnDestruct;
}

public function delete(DeleteType $type): void { . . . }

$file->delete(DeleteType::OnShutdown)

It would limit a possible 2.0-release to PHP8.1, but migratiting from PHP8 to PHP8.1 should be fairly easy. PHP7.4 will reach end of life (no security fixes) in half a year anyways, so I think it's not really concerning dropping PHP7 entirly from Releases. I mean, people really depending on PHP5/7 can still use old 1.x releases, they're not gone.