Re: Fw: setStub inconsistencies - dubious patch attached

Steph Fox wrote: > Trying again on-list (hello Pierre). > > - Steph > > ----- Original Message ----- From: "Steph Fox" <steph@zend.com> > To: "Greg Beaver" <greg@chiaraquartet.net>; "Marcus Boerger" > <mail@marcus-boerger.de> > Sent: Thursday, January 31, 2008 4:30 AM > Subject: setStub inconsistencies - dubious patch attached > > >> Hi both, >> >> I went to look into the tarmaker issues earlier, and now have >> shitloads of >> questions about the implementation of setStub(). That's what happens when >> you send me to look at failing tests, see... >> >> 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 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. >> 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. >> 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. >> 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. 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. Thanks, Greg