php / php-src

The PHP Interpreter
https://www.php.net
Other
38.23k stars 7.75k forks source link

Add an iterface to throw that will not collect debug_backtrace on "new" call #13301

Closed 6562680 closed 9 months ago

6562680 commented 9 months ago

Description

Look for javascript. It allows to throw anything. String, array, object. Most of times its because classes is a sugar for js most of times.

But.

In PHP even already designed an methodology that data validation should be performed as much earlier than possible and then you have to use valid DTO to escape validation stuff inside service code.

Old article of Martin Fowler also explains about "errors" is not the "exceptions". And next i will explain why...

I've written tonns of API's and i've noticed that sometimes errors is returned in responses (not exactly one error), errors could be in request preparation, even then response couldbe misformatted and produces errors that is not inside response directly.

While you working with the queues - your common case "save all errors to queue task cells" even if you process them in batches. Batch processing forces you sometimes to (map) tasks, sometimes to (reduce) them to aggregations, sometimes (reduce) out from aggregations. That way some errors could be [copied] to few records, some could be [separated] by types/indexes/uuids.

Common solution from php community is log tasks using monolog, usually to file/remote_system(sentry, email), could be implemented array store, but uncovered is the case "in that moment you cant decide what key you have to use on upper level".

Would be great to catch "error batch" and create class for, but one thing is here, and thats not only problem in PHP.

Imagine you try to send bulk data with 1 000 000 records via CSV file. Each record could return warning or error. If you write code that throws error or error batch from each record - you solve the task, but "new \Exception" collects backtrace on create. So you lost 0.05 seconds on each from million rows in worst case. Other class cannot be thrown!

You always need to write your own errorBag class and errorBagStack class to implement it like that:

<?php

function fn_a() {
  $errorBagCurrent = _error_bag();

  $errorBagChild = _error_bag_push();
  fn_aa();
  $errorBagPop = _error_bag_pop();
   // var_dump($errorBagPop); // > contains 'Hello'
   // var_dump($errorBagChild === $errorBagPop); // true

  _error_bag_merge_as_warnings($errorBagPop, $mergeKeyName = 'fn_aa');
  // var_dump($errorBagCurrent); // > contains [ 'fn_aa' => 'Hello' ] in some structure like "{group_1}|{group_2}" to filter later

  if ($errorBagCurrent->hasErrors()) throw new \Exception('Fatal'); // > never happen, only warnings inside current bag
}

function fn_aa() {
  _error_bag_error('Hello');
}

$errorBagCurrent = _error_bag();

_error_bag_push();
fn_a();
_error_bag_merge_as_is(_error_bag_pop(), 'fn_a');

var_dump($errorBagCurrent); // > contains [ '{fn_a}|{fn_aa}' => 'Hello' ]

Btw, javascript way is good for low-level, but not ideal, because while you work with chains usually you always have TWO returns - errors and result, because catch statement could replace error to result. So most of cases returned arrays with [0,1] keys where 0 is an error/error-bag, 1 is a result, if 0 key is not empty, 1 key is ignored.

Additional case is "there's warnings and errors". Warnings should be stored or printed, but should not stop current function. So errors from level 2 could be merged as warnings on level 1, and the check ->hasErrors() returns false, but the check isNotEmpty() returns true.

Please, implement some feature to the language core that covers all cases above.

I dont know what solution could be the better, but, am using above printed _error_bag() for now

damianwadley commented 9 months ago

It allows to throw anything. String, array, object. Most of times its because classes is a sugar for js most of times.

I work in Javascript a lot and I will 🔫 anyone who tries to throw anything that isn't an Error. It just causes too many headaches.

Please, implement some feature to the language core that covers all cases above.

It sounds like what you want is really simple: not exceptions.

I don't want to go into a whole philosophical treatise about what exceptions are, but the gist of it is that exceptions are for exceptional behavior - things that aren't supposed to happen in your application but do anyway. If you want to report failure with something, and that failure is not an error or symptom of a problem that requires troubleshooting, then an exception is not the answer.

Imagine you try to send bulk data with 1 000 000 records via CSV file. Each record could return warning or error.

You answered your own problem right there without even realizing it: "each record could return warning or error".

class TaskResult {
    public bool $success;
    public ?int $severity;
    public ?string $message;
}

Return an instance of that, or if you want to get really OOP-edantic then use three classes (Result base, Success and Failure children), and you're done.

Or if you want something even more lightweight, simply return string|true. Or string|null. Or some object |null.

I personally like some of Rust's idioms, namely Result<> and Option<>. The language doesn't have exceptions, mind you, but support (preferably low-level and optimized) for the former would be especially useful here.

Regardless, thrown exceptions have a specific use case, and if your use case does not warrant throwing exceptions then don't do that.

6562680 commented 9 months ago

It allows to throw anything. String, array, object. Most of times its because classes is a sugar for js most of times.

I work in Javascript a lot and I will 🔫 anyone who tries to throw anything that isn't an Error. It just causes too many headaches.

Please, implement some feature to the language core that covers all cases above.

It sounds like what you want is really simple: not exceptions.

I don't want to go into a whole philosophical treatise about what exceptions are, but the gist of it is that exceptions are for exceptional behavior - things that aren't supposed to happen in your application but do anyway. If you want to report failure with something, and that failure is not an error or symptom of a problem that requires troubleshooting, then an exception is not the answer.

Imagine you try to send bulk data with 1 000 000 records via CSV file. Each record could return warning or error.

You answered your own problem right there without even realizing it: "each record could return warning or error".

class TaskResult {
    public bool $success;
    public ?int $severity;
    public ?string $message;
}

Return an instance of that, or if you want to get really OOP-edantic then use three classes (Result base, Success and Failure children), and you're done.

Or if you want something even more lightweight, simply return string|true. Or string|null. Or some object |null.

I personally like some of Rust's idioms, namely Result<> and Option<>. The language doesn't have exceptions, mind you, but support (preferably low-level and optimized) for the former would be especially useful here.

Regardless, thrown exceptions have a specific use case, and if your use case does not warrant throwing exceptions then don't do that.

  1. Please, understand my experience with more that 10 years in php (everyday).

  2. I do not ask "remove exceptions". Exceptions for me is a solution for two cases.

    • Force program to stop if programmer/user made mistake with input that cannot be fixed (only because we dont have UI layer, otherwise i will catch it on render)
    • Force program to stop if i made mistake in my code and just miss it (or i just dont know how to do here, i left exception to decide later), so is a tool to speak with me but later, or with other developers who will work on my code. I asked for implement secondary thing for cases that does not require exception.
  3. I dont understand is you want to listen the point, but first time i've met that "simple returning TaskResult" is bad - once i've just created API with 500 methods. And then, they decide "just refactor it please". I have to change tonns of signatures in nesting, one method returns errors, that should be merged with parent ones then return, then merged, then return, then merged then return, all because i need in last level

    • save certain pack of errors to certain db cell
    • save all errors to file log
    • print all errors and warnings to json in user output if he has access to

Its not the case of "just fix your code", that stuff could be enabled or disabled on certain function without changing the code. "Just rewrite your code" never possible in real world, because business dont want to pay for "rewriting". Tired of hiding required work under their requirements.

I use exceptions. I use it often. Users hates exceptions. They dont want the program that accidentaly stops. No, is not a nature of PHP, php can work as daemons, can work asyncronyosly, can process batches, can read gigabytes. Its not a "this language vs other language", both cases are useful, when it needed.

Exceptions do two responsibilites. Raises error and stops program (actually one - stop the program, try/catch does second responsibility). Working in real world you often need to catch error and continue on next level. And, as an addition - there's main problem - every new \Exception spents time to collect the trace.

I do not need the trace, so \Exceptions is not my case. And i dont want to rewrite 200 thousands lines of code implementing new return object because "that is truly". I want to get errors immediate here, fixing concrete place and catching that fix outside where i want without changing logic of stopping proccesses, foreaches, generators, batches after the code is ready (as a case).

And common, chaining with promises still cannot work with exceptions because catch is a thing to catch and return correct result. How it should looks? Catch exception then return array with the exception and correct result? Ok, next if should decide what to do with that exception? Return again? Merge with current ones? Hide? Log? All? Throw method succesfully return one Task, and then should be converted to message, that is logged and saved for api output, so global context works with local context.

And requirement to show handled case conceptually never disappears. We can equate that "controller layer methods requires to return errors and results" but "service methods could return errors or throw them", in middle we sometimes use catch, sometimes - error bag. Trouble is in that you cannot simple throw somewhere because you still need to change other high-level function to catch, then you create time-spending error constructor, and then you still needed error bag to collect catched errors, and you still need some way to return collected. So all of your functions accidentally could start to require changing return signatures and force you to create class for each one. What a surprise.

A lot of problems just about "we dont believe error bag" or at least "we cant create class that wont collect trace to reduce timelosses"

Btw, thats the story of json_decode function that initially returned null on fail and collected error to json_last_error memory cell. People hate that "strange behavior" and asked to implement "throw exception" flag. And whats now? He catched exception and still return null once catched. Correct way? Return "fallback value" and return reference to error. Fallback could contain API schema, reference could contain message and code, maybe source to debug. And that case could appear in any function you write depends on business (most of times - handle speed)

6562680 commented 9 months ago

Look how can i do with code almost without refactoring:

<?php

function fn_a() {
  _error_bag_push();
  $result = fn_aa();
  _error_bag_merge(_error_bag_pop(), 'fn_aa');

  return $result;
}

function fn_aa() {
  $results = [];
  for ($i = 0; $i < 10; $i++) {
    $eChild = _error_bag_push();
    $results[] = fn_aaa();
    _error_bag_merge(_error_bag_pop(), [ $i, 'fn_aaa' ]);

    if ($eChild->hasErrors()) continue;
    // ...logic
  }

  return $results;
}

function fn_aaa() {
  $results = [];

  for ($i = 0; $i < 10; $i++) {
    $results[] = fn_aaaa();
  }

  return $results;
}

function fn_aaaa() {
  $result = null;
  $e = _error_bag(); // i get current bag because i decide later inside this context

  // > ..,logic, $result = 1;
  _error_bag_error('Hello'); // this code save error to current error bag, if i didnt receive it 2 lines above - it will save to parent, otherwise to his parent, or to dev/null if no wrappers

  if ($e->hasErrors()) return null;
  return $result;
}

$e = _error_bag();
_error_bag_push();
$result = fn_a();
_error_bag_merge(_error_bag_pop(), 'fn_a');

var_dump($e, $result);

As you can see i changed certain places, my signatures left untouched. Once i need errors, i inject error context where i need that feature, and then catch where i need, i do not require to catch it on each level.

I've got all errors of all children anywhere i want, i can search by error bag by group/key, can change severity, can check severity, also i can collect few errors to one object and decide - drop it out, log right there or save for the future (parent level wrapping).

Better could be only tag attachment to certain errors to allow anywhere search by error type without nesting. And that case is not simple too. Searching by key most of times needed when you reduce batch from 1 to many, and there you dont know key you attached directly on error place, most of times you attach key after you searched by another groupKey.

Btw nesting is required while debugging. Same as exception, and i decide what part of program i should stop, ignore or continue adding simple lines without wrapping scopes, writing exception, return classes, collecting trace. Trace is collected on the fly exactly where i decide.

First build of that error bag was a trait that allows to run any method like "[ $e, $result ] = runWithErrorBag('fn', ...$args)", but it changed code to unreadable one.

Correct way is running like in SQL "with recursive" -> "with error bag { ...code } merge to $key", i mean language construct

Or even simplier. language construct raise that raises error. Same as throw but dont break the code right now. try/catch block could catch this raise to reraise again if you need to merge and mark with tag. That catch always gives you array(nested) because raises could be more than one. Raise have to work with arrays to merge and nesting. Errors could be any type. string (message), array (interpolate) or object (incapsulate), and never be \Throwable.

damianwadley commented 9 months ago

I'll be honest and tell you that I stopped reading early into your responses, so:

For a feature request like this, you'll need to go through the RFC process. You can find more about it here, which will tell you that the first step is to raise the subject on the internals mailing list for a preliminary conversation.

6562680 commented 9 months ago

OOP is like hard-training in military. Its required, but it will useless once passed. All processes of the world are async/parallel and all processes require changes, not reengineering, and functional programming is more smart for the case (chaining/pipes/tasks instead of classes/methods/incapsulation).

But if you didnt know how to write OOP, you will break functional programming process with bad code. Thats why "believe in OOP" is not a good idea for me.

Javascript going to back to OOP because they had missing required step. PHP is stuck on training step.

Python just used old good generator idea and... change world accidentally. But it allows to write several ways the same, so there's less possibilities to cowork and collaborate.

Go just implement own process manager to make fast parallel processing, i mean - [ nix forking / windows process creation ] wrapper in language core, and its still alive.

Girgias commented 9 months ago

I'm really confused by what is being written here.

I'm not sure if this is just a language communication thing or whatnot.

But exceptions is not the tool that you want or need here to solve your problem as far as I understand. Exceptions interupt the control flow and should not be used to deal with expected error conditions. Use a enum and union types return to indicate the status and provide more information rather than wanting an exception that doesn't generate a backtrace.

6562680 commented 9 months ago

Theoretically talking "is possible". Above i explained that is possible, and even explained how to solve "dont touch". But why not just deny ideas because of "this is idea and we dont want to read (I'll be honest and tell you that I stopped reading early into your responses) YOUR ideas" (community responsibility, time, complexity, select any reason you want)

Main point - reduce redesign time and prevent possible new refactoring errors once signature changes, prevent unexpectedly fall all existing logic because somewhere else your code was used with old return types. OOP is a thing of ideal world. It declares that you CAN design system that is stable and never changes. Its a mistake.

You should be able to "dont touch existing logic" and implement error collecting after your code is in production. Return types, incoming and returning data should not change without business needs. Error collecting is not a business requirement, but developer one.

Girgias commented 9 months ago

Again, I don't understand what you are saying here.

Also, if you have badly designed something, then yes you should redesign it and not ask the language to change behaviour because of convenience.

As to why not just say "we disagreed with your idea", this is the bug tracker. Not a feature request or feature discussion tracker. These things happen on the internals mailing list, because more people are subscribed to this, as this is its purpose and not to the bug tracker.

If you think OOP is a mistake, then don't write OOP? You can write pretty functional code in PHP.

Finally, do not edit your response 400 times, take the time to write a well formulated response and send it once. No need to hurry to submit a reply.

6562680 commented 9 months ago

Thats the point, mam. Developer ALWAYS has a bad design if developer detected error later. Its not a MOMENT of design. It is a time period. Developer counts he's done. But tomorrow he see - he doesnt. And now he has to redesign and somebody must to pay for. Ideal world.

Real world - "we wont pay you for your mistakes, you can do it at night or when you want". Thats why its always developers problem, not business. Nobody listen developer complains about "he need to refactor".

As to why not just say "we disagreed with your idea", this is the bug tracker. Not a feature request or feature discussion tracker. These things happen on the internals mailing list, because more people are subscribed to this, as this is its purpose and not to the bug tracker.

Really? I accidentally write to feature request branch?

Btw, its not my problem. I've solved that my way. If others should do it their own way made my mistakes again - lets go!

iluuu1994 commented 9 months ago

This discussion doesn't seem very productive, so I'm closing this issue. As has hinted at by @damianwadley, it seems you'd like to have stack unwinding as normal, non-exceptional control flow. This is not really what throw/try/catch are designed for. This is a big feature request and shift in mentality, and as such needs to be discussed with the community first, i.e. the internals mailing list.

Please send an e-mail with a concise but understandable description that communicates the problem you're facing, and your proposed solution to it. Parts of this conversation were very hard to follow.

As you're probably aware, you could also already achieve this by wrapping your value into an Exception. So I'm guessing the community will ask for some good argumentation on why this feature is worthwhile.

class Box extends Exception {
    public function __construct(public $value) {
        parent::__construct();
    }
}

try {
    throw new Box(42);
} catch (Box $e) {
    var_dump($e->value);
}

But why not just deny ideas because of "this is idea and we dont want to read (I'll be honest and tell you that I stopped reading early into your responses) YOUR ideas" (community responsibility, time, complexity, select any reason you want)

I presume the reason @damianwadley stopped reading is because of the condescending undertone in your messages. Maybe language barrier is at fault here. Also note that Damian is an unpaid volunteer. All the effort he puts into the bug tracker to help people (and the PHP project) is out of goodwill.

6562680 commented 9 months ago

This discussion doesn't seem very productive, so I'm closing this issue. As has hinted at by @damianwadley, it seems you'd like to have stack unwinding as normal, non-exceptional control flow. This is not really what throw/try/catch are designed for. This is a big feature request and shift in mentality, and as such needs to be discussed with the community first, i.e. the internals mailing list.

Please send an e-mail with a concise but understandable description that communicates the problem you're facing, and your proposed solution to it. Parts of this conversation were very hard to follow.

As you're probably aware, you could also already achieve this by wrapping your value into an Exception. So I'm guessing the community will ask for some good argumentation on why this feature is worthwhile.

class Box extends Exception {
    public function __construct(public $value) {
        parent::__construct();
    }
}

try {
    throw new Box(42);
} catch (Box $e) {
    var_dump($e->value);
}

But why not just deny ideas because of "this is idea and we dont want to read (I'll be honest and tell you that I stopped reading early into your responses) YOUR ideas" (community responsibility, time, complexity, select any reason you want)

I presume the reason @damianwadley stopped reading is because of the condescending undertone in your messages. Maybe language barrier is at fault here. Also note that Damian is an unpaid volunteer. All the effort he puts into the bug tracker to help people (and the PHP project) is out of goodwill.

Yes, it requires the not-only-the-community discussion.

As i wrote, throwing errors as exceptions (wrapping) will cause timelosses to trace collection, thats why its almost a BUG. But that bug could be solved just with notice you wrote - "thats is not the design of", so then its a feature request.

I agree with the topic that it would not be my idea, but it would be functional to do

Main troubles:

  1. timeloss on collect trace (directly on call new() with \Throwable)
  2. unable to refactor without changing signature or return value
  3. needs to be catched otherwise all other logic will fall

So the solution is adding "non-breaking" throwable that is autocollected only if you catching it (otherwise does nothing) Once catch - you could put it into "top" (in relation to place you throw it its upper, but in relation to place you catch - its current) level bag (rethrow) with own nestings / tags. Nesting is needed on debug, tags is needed on search/merge to save groups to different db cells depends on queued task.

Thats all.

6562680 commented 9 months ago

image image image

iluuu1994 commented 9 months ago

@6562680 internals@lists.php.net is the list you're looking for. You need to subscribe to it first before sending an e-mail to it.

6562680 commented 9 months ago

@6562680 internals@lists.php.net is the list you're looking for. You need to subscribe to it first before sending an e-mail to it.

Finished. Thanks for providing me with.

6562680 commented 9 months ago

error-bag.zip

For those who want. And "not really confused of language barrier" with that person who say "as is" without "principles".

https://github.com/6562680/error-bag

6562680 commented 5 months ago

I'd just remember that PHP has __destruct() method and finally i found the use-case for.

This method is called when object, that is created inside function is dying, so it is exactly creating function-wrapped context.

If i create error bag inside function and add listener on destruct to close this error bag - even if exception is thrown inside deep functions - error bag will be cleared, for example to log file.

It means - with __destruct() method PHP allows to create function-based context that is auto-removed if this concrete function is finished of breaked out.

I will update the package to keep that thing useful.