Hi Bob,
Thanks for the feedback, the replies here also cover the feedback from
Jordi and Ignace.
> Why have you chosen a big list of constants rather than enums?
> I think we should push for enums where applicable, meaning enum
> StreamErrorMode, enum StreamErrorStore, enum StreamError.
>
Ok I will change it. StreamErrorMode and StreamErrorStore should be
straightforward. The code one should be probably named StreamErrorCode (to
not interact with what Jordi proposed) and think it will need to be backed
as there should be a straightforward way how to compare it with exception
code. I might need to extend the gen stub as I'm not sure it allows macro
import for enum value like for constants or I will need to come up with
some mapping. I just don't wan to duplicate the numbers. I will figure
something out.
> You define what a terminal error is, but does it correlate to specific
> error codes? Like, is a given error code always terminal or not
> terminal? Or can some error codes sometimes be terminal or not terminal
> depending on context?
> I've tried to look at the code, but I don't quite understand it, e.g.
> php_plain_files_rename: When chown or chmod fail, the error is terminal.
> When the copying fails (did you forget to adapt php_copy_file_ctx?),
> that's non-terminal, though. Reading through the code, I have the
> feeling that operations which do not call out to other operations, which
> can error, are terminal. And all others are non-terminal. E.g. failing
> to open a file is terminal. That the copying where the file opening is
> part of, failed, is non-terminal.
> And then *additionally* some operations, which don't prevent success are
> also marked non-terminal - squeezing it into the boolean for the purpose
> of not erroring.
> Which makes some sense to me, but the description in the RFC is really
> confusing here. And the meanings are muddied.
>
So currently the terminal / non terminal does not attach to code but it
could be done. The current logic is really just supposed to reflect what is
there already so the exception can be thrown in places where the function
would fail. So the errors after which the function fails are terminal and
the errors where the function still returns success are non terminal.
Thus, if I understand that correctly, there should be an unique mapping
> from error code to terminal. If we were to go with enums, we could make
> it trivially a property of the individual enum values instead.
>
I think making it property of enum make more sense. Then I can drop that
boolean property.
> Should StreamException be attached the non-terminal errors which are
> caused by the terminal errors? I.e. StreamException clearly is
> STREAM_ERROR_CODE_PERMISSION_DENIED for example, but it happens during a
> copy, so, should the information that a STREAM_ERROR_CODE_COPY_FAILED
> was caused by that, also be present on the StreamException?
>
I think I get what you mean from the user point of view but I'm not sure
how could this be reasonably linked.
I could maybe check during stream error if there is EG(exception) it's a
StreamException and if so, I could the update it with this error (e.g. some
property that would hold those error can could be retrieved). I guess it
could be potentially extended for terminal errors as well so it could have
method like getAllErrors which would return all errors because sometimes
there can be more terminal errors and some of them might get lost (last one
will be added). However this would work just for exception so maybe more
generic solution is needed here (see below).
> Are stream errors which happen as part of a fully wrapped functionality
> marked as wrapper error as well? Like if the server sent an invalid TLS
> response to a file_get_contents("https://...";) operation, making the
> whole operation fail? You obviously don't have access to the
> intermediary internal stream resource there, where the stream error is
> attached.
>
I actually thought about this yesterday that it's not ideal that some
errors might get lost in this way so I think it should go to wrapper errors
as well.
> Is this what the logged errors section is about? Is that section talking
> about "php_stream_display_wrapper_errors", which can concat some errors?
> I see that this logic is mostly retained.
>
Yeah the logged errors is an existing mechanism for grouping errors to a
single one (concatenating message). The display is a bit messy because it
separated them by new line or <br/> (if html errors enabled) but I just
kept it to limit BC impact.
> Do I understand the implementation correctly, that user error handlers
> can be called multiple times for a same operation? E.g. when copying a
> file, you'll get first a PERMISSION_DENIED then a COPY_FAILED? Should
> these not be somehow merged? Especially with global user_error handlers,
> these cannot be aware of the fact that the permission denied is part of
> anything else - at least not until possibly a second user_error
> invocation happens.
>
This is somehow related to the exception error grouping as it requires some
sort of linking between the errors.
I guess to make this work we would need some top level wrapping similar to
what logged errors do but instead of concatenating, it would group the
errors in a better way which could then be used in exception or handler.
This would, however, increase the complexity significantly as it is
possible to nest the stream calls (e.g. stream calls in notification
callback or in the new error handler) so there would probably need to be
some stack. Also I'm not sure about the API. Should we pass always array of
errors to the callback (which would mostly have just one element anyway or
somehow chain the StreamError object (something like previousError()) and
what message should be used in exception (e.g. last error and then helper
function to get all errors)?
I'm willing to look into this but would like to know if this is something
important for user space and what is the usual flow how could this be used
in frameworks and so on. In other words I would like to get more input to
make sure that this is really needed as it requires quite a bit of effort
to do.
> Also, is the internal resource used for e.g. file_get_contents() leaked
> into user error handlers? I'm not sure whether that's desirable. If
> that's intended, that's fine too, I think.
>
>
Currently yes. I need to actually double check the consequences of that as
I didn't really think about internal resources.
> Further suggestion on stream_get_errors() - why not return a proper
> object instead of a loosely typed array? Proper typing, and creation is
> actually faster. Also "docref" is so... antiquated? maybe just
> "function" and have it contain the function name of the current operation.
>
>
Yeah I will go for it and introduce StreamError as Jordi suggested with
slight modification to use enum and dropping terminal and docref.
> Does stream_get_errors() ever reset? Looking at the code it doesn't seem
> so. It will just forever grow. E.g. fwrite() will report an error every
> single time when the buffer is full and a write is attempted. If errors
> aren't freed, that's just a memory leak (as long as the socket lives).
> This is a serious flaw.
>
> Lastly, stream_get_errors() is overkill in a lot of cases (especially
> when using fread()/fwrite() on simple sockets). I just care about the
> last error code (there's just going to be one anyway). I'd love having a
> stream_get_last_error() returning just the code.
>
>
I guess this makes more sense but it kind of also implies the top level
wrapping as it should contain all errors in the call. Or alternatively it
could be just a ring with limited number of errors be behave in the same
way like openssl_error_string. I guess it depends if we go with that top
level wrapping.
Cheers,
Jakub
