Re: Fw: setStub inconsistencies - dubious patch attached

Hi Greg,

To start with there are exceptions all the way through that method, which seems like a pretty bad idea given that it's supposed to return a Boolean value. Shouldn't there be a bog-standard E_WARNING on failure? And why would you want to throw an exception - which for most mortals causes a fatal error - just because you couldn't set the stub to anything other than default? Also, when you rely on exceptions, calling setStub() with the wrong data can result in a completely empty phar because there's no way to return FALSE. So I don't think exceptions work well there, given that this code is supposed to be easy for other people to use anyways.

There are two ways of accessing phar archives: the stream wrapper and the Phar object. The stream wrapper has a context option for setting the stub, and any problems there cause E_WARNING to be raised. All problems with the OO interface use exceptions for error handling. The idea is that you encase your OO stuff in try/catch, or use the stream wrapper for simpler stuff. I don't want to change this behavior, it would mean rewriting all of the OO stuff, or having an inconsistency.

I have a suggestion you may or may not like. Would it be possible to have an INI_ALL directive to use exceptions or standard errors? I'd personally have it with standard errors by default except in cases where there would be a fatal error anyway, which basically restricts exceptions-by-default to failures in the constructor. I don't think it matters greatly that the internal code is OO... Point being, if you're aiming for this to go into the PHP core, exceptions are literally the exception and not the rule there.

I don't see the point of passing in a length to setStub(), optionally or otherwise - is there some reason for not just passing -1 to phar_flush()? or is it a BC thing? (I can't remember if/how setStub() was before.) It'd be easy enough to put it back, I just took it out because it looked so pointless.

The point is that you can pass in a file pointer, seek to where you want, and only use part of it for setting the stub. This is useful if you have limited control of the thing you're passing in.

OK, so I'd put that back in.

Another problem is the quiet parameter parsing for resources, which means that any parameter-parsing errors will mention the single-parameter string option and nothing else. Confusing, if you happen to be working with file pointers...

This can be fixed best by checking the type of the first parameter, and using a better error message.

I think you wrote this before you looked at the patch. It's actually nigh on impossible to give a helpful error message in PHP 5.2 code, but it's readily do-able in PHP 5.3.

Finally, there's an error 'unable to read from input stream' which I couldn't reach to test. I'm really not sure what you have to do to get that warning, so I left it in.

That's a "covering all possible bases" error. I think you can trigger it by passing in a closed resource.

Have you ever managed to trigger it? I'm curious because I tried just about everything I could think of and still couldn't trigger that error.

So - the attached version's a fair bit longer than the original (not least because I like lots of ws) but it does at least return TRUE when the method's had the intended effect, FALSE otherwise, doesn't wreck the phar in the process, and tells you precisely what went wrong if anything goes awry. It's probably not very fashionable tho'. Let me know what you think.

I like the param parsing a lot, it makes the errors much clearer, but as I said, throwing exceptions should be put back in.

The param parsing's 5.3 only, I didn't realize this until after I'd posted the patch. Then again, so's the stuff Marcus put into CVS last night, no? so I guess this needs to be taken into consideration.

One other issue that I think you uncovered Steph is that phar should prevent creation of the archive unless at least 1 file is added, as we can end up with a phar containing just a stub, which will not load up at all. So, we have a few more kinks to iron out, that's certain.

Yeah, I'm all too aware of that. But it's definitely getting there :) - Steph

Thanks, Greg