Source
zimzat•

Using a constant for the message causes the placeholders to be declared separately of the values, meaning you would have to jump between the constant and the usage to figure out what each sprintf argument needs to be. Was the second parameter a %s or a %01.2f? Is it referring to the whizzywig or the flux capacitor? 🤷‍♂️

new self means that the exception can't be extended. Your linked source class is final so that is less of a concern, but also means that every exception you write has to copy-paste these methods.

The withFormatAndPrevious placement of $previous in the middle between the format and values is a code smell. Either it should be ($previous, $format, ...$values), or it's an indicator it shouldn't be done that way.

I'm with u/kafoso on the usage of static methods to create exceptions. It adds a layer of indirection during development and debugging. In this case it doesn't add any benefit to the process: No strongly typed inputs, anyone can do CurlException::withFormat('Random arbitrary string and ' . $concat), etc. If you're going to go down that path then make it more readable and functional by creating a separate static method for each type of exception:

public static function createFailedRequest(string $url, string $error, string $info): self
{
    new static(sprintf("Unsuccesful request to url: %s\nError: %s\nInfo: %s", $url, $error, $info));
}
  1. Correctly typed inputs.
  2. Descriptively named input variables.
  3. The message placeholder is adjacent to the value.
  4. No internal constants being publicly exposed.

If you extrapolate that to a method that only returns the formatted message string formatFailedRequest then you can easily reuse it anywhere while minimizing the layers in the backtrace, and allow using the standard $previous and $code without having to reinvent the wheel.

7
htfo•

Using new static() forces the constructor to be part of the public interface which is usually not what you want to do, as the constructor is supposed to be an implementation detail of a concrete class. There's a reason why most linters have rules against its usage.

1
ltscomOP•

I prefer composition over inheritance so using final classes and self is fine by me. If I needed multiple Exception classes in the same library, I'd extract the with methods into a trait to keep it DRY.

I don't understand the point about code smell though in terms of argument order, what smell is it exactly?

There isn't anything to prevent someone passing arbitrary formats into the with methods, but at that level I trust the developer (me) not to deliberately try to work around the expectations.

The idea of having specific methods for each message is a nice one, but I would still keep the formats as constants so that they can be easily used when testing. Alternatively there could be a public method to generate each message string but it starts to create quite a lot of public methods on the Exception.

-1
zimzat•

I don't understand the point about code smell though in terms of argument order, what smell is it exactly?

That the arguments aren't in the order that they are related by. Their relation goes a1, b, a2 instead of a1, a2, b (since the variadic argument has to be last) or b, a1, a2 (which keeps the variadic argument last).

There isn't anything to prevent someone passing arbitrary formats into the with methods, but at that level I trust the developer (me) not to deliberately try to work around the expectations.

Classes are public. Anyone can create an instance of one, whether in your code or otherwise.

If you have an open source library then the pattern does not enforce that behavior so you're more likely to get pull requests where people don't follow that pattern since it's not enforced, it's not explicit, and it takes extra effort to do it the way you do. This may also be a concern if you're working in a team, or on-boarding new developers not used to this pattern.

When searching by a message string (in case the trace is missing) leads to the exception first, then everywhere that constant is used has to be traced backwards. This is the same indirection added by creating the exception elsewhere from the error itself.

If you're using the message in your tests then the text should be duplicated. Your test could incorrectly implement the sprintf parameters in the same way that the code does, the test would pass, but you wouldn't realize the final text ends up reading The 1 cannot be status. when it should be The status cannot be 1..

throw new CurlException(sprintf('Request Error: %s', $error), previous: $e);
IDEs like PhpStorm will show an error on this line if there are more arguments than placeholders or they're the wrong type

throw new CurlException(CurlException::formatRequestError($error), previous: $e);
The argument and type is explicit, the message itself is DRY (and can be called in a test, if you must), and IDE can check placeholder usages internally.

throw CurlException::withFormatAndPrevious(CurlException::MSG_GOES_HERE, $e, $error);
Placeholders are invisible, message is elsewhere, it's not obvious what's happening internally, the $e is in the middle between the two related data points, and it will only be a run-time exception (while handling an exception) if there are too few or too many arguments.

1
ltscomOP•

I do accept that using a method to generate the exception message is cleaner from a stack trace point of view and it is also provides more safety in terms of being able to properly type all the arguments.

I think it is an improvement on the general concept.

I would like to avoid passing in arbitrary strings to the exception as it makes things harder to test/refactor and fails my lazy developer threshold, so that rules out using sprintf directly.

1
kafoso•

There's a consensus, that with... methods largely are reserved for immutability. These, I'd argue, are create type methods.

For exceptions, though, I'm personally largely against using static methods for construction: It will create unnecessary clutter in stack traces.

Nothing wrong with having a method in the exception class for generating the string containing the error message and then passing said string in the constructor. This goes for other arguments, too, e.g. $code.

Lastly, by omitting arguments like $code and $previous, you go counter to the most basic of functionalities of exceptions.

The intent is good, but it needs tweaks.

3
ltscomOP•

thanks for comments. Exceptions are by their nature immutable so I'm not too worried about that aspect

Ommiting code and previous is by choice and where previous is required, there is a separate withFormatAndPrevious method. Maybe I should have mentioned that in the post https://github.com/LongTermSupport/microdeps-curl/blob/86bb3e41323b7d6521d9f6b23dd5d32b70ee8a76/src/CurlException.php#L32-L35

edit:

I decided to edit the post to bring in the withPrevious method and also explain the private constructor to try to enforce using predefined message formats.

3
kafoso•

I could have been more clear about the usage of with methods with immutability. Categorically, these methods are not supposed to be static in immutable classes.

Example:

/**
 * Returns a clone.
 */
public function withFoo(string $foo): static
{
    $clone = clone $this;
    $clone->foo = $foo;

    return $clone;
}

To clarify about the stack trace:

2
ltscomOP•

Fair points on the wither. I'm not someone who subscribes to overly firm rules about this kind of thing though.

The stack trace points are more interesting and I can see the value in trying to avoid having the top of the stack trace be the creation of the exception instead of the point where the exception is thrown.

1
noccy8000•

It may be a good idea to at least pay attention to conventions between humans. A lot of people follow the same ideas to make their code easier to understand. Like how cars are all started in the same fashion. If you put a key in the lock in the steering column and turn it, you expect the car to start, not the windshield wiper to go off or the trunk to open.

Or you could make a library where RTM is a requirement. Tho it may become a problem if your code is to be used in any other project. There are projects out there that use all lowercase namespaces, which is fine, but it looks awful when used alongside structured code like in Symfony projects.

tl;dr The better your code follows conventions, the easier it will be to integrate.

1
slepicoid•

Unfortunately storing the message in a constant and then using that constant in your test case is asking for problems. If someone changes the value by accident, your test will still pass which would be wrong. It's simply impossible to have a test for constant value that does not break when the constant value is changed. You can move such value to constant and you can use that constant in your tests. However not in the test of the constant value. You should add a separate test that the constant has the value that it should have. The reason being that by introducing the constant you introduced complexity. Formerly the public interface of your exception made a promise that the message will have certain value, the new one also promises that there will be a constant with certain value. Two promises, two tests.

1
ltscomOP•

I don't see this as a particularly big risk in my case. The actual message string is not overly important. From my point of view, the constant name is whats important and I need to ensure that the correct exceptoin message is being emitted. If there's a typo in the actual message string, then I can easily fix that in the constant and no further code changes are required within tests etc.

Testing that a hard coded constant is hard coded to the correct string by also hard coding that expected string is valid but excessively verbose for me. I'm lazy and like refactoring to be as easy as possible.

1
slepicoid•

Honestly I never write tests for constant values or exception messages. I was just trying to point out that you already had such a test and it got invalidated when you refactored to use a production class constant in the test. It's not just about constants thou, it's a general problem. Anytime you use a production code as part of your test, you better have that code tested as well... It's just that with production class constant it comes down to the trivial test of constant having the right constant value...

1
ltscomOP•

I suppose this is a balance between testing best practice and keeping it DRY/being lazy

1
slepicoid•

Fair enough. Sorry if my initial comment sounds more like "you're in big trouble" rather then "here's something worth pointing out" which it should have.

2
NJ247•

Honestly I never write tests for constant values or exception messages.

I've started doing that as well. I feel testing an exception message is a unit test on it's own and I should just test that the exception was thrown based on the subject under test.

1
ltscomOP•

I got into the habit of testing the exact exception message when I found that the wrong exception was being thrown due to an expected error, but tests still passed.

Testing the correct message is a good way to be sure its definitely the exact exception you expect.

1