Replace with_merged_former_errors by validate_and_merge_errors

[NobodysNightmare]

The main problem with the previous method was that it only worked if the contract being validated inside the block was sharing the same errors object as the contract calling it. The old implementation was incapable of incorporating errors from a contract that had its own errors object. Sharing of the errors object is pretty common among our contracts, because most of them just delegate the errors method (among others) to the model being validated.

In my opinion it's a code smell that our contracts use the errors of the model they are validating as the place to store their validation result, however I didn't dare touching this yet, because changing that would certainly be a huge change (e.g. because it means that validating the contract will change the model being inspected). I can also imagine that some places rely on it for historic reasons (because using "classic" validations also puts the errors on the model). However, I think that no two contracts should RELY on using the SAME errors object, just because the way we implemented them happens to cause them to share the same object.

What are you trying to accomplish?

This PR was coupled out from #17838 to be able to discuss and test this change separately. In #17838 I want to add a ComposedContract which is essentially a way of combining multiple contracts into one.

That class is not reusing the errors object of the model under test, because that would introduce an unintended coupling between the contracts that I'd need to work around.

[NobodysNightmare]

What makes me sad is that the implementation of the new method should be

contract.validate
errors.merge!(contract.errors)

This is not possible, mostly because we need to deal with the fact that we might be sharing the errors object with the merged contract (or a contract called by the merged contract). That I discovered a bug in added? along the way doesn't help the amount of necessary method comments either.

[mereghost]

I think that this was on purpose due to how dependent services worked. You might want @ulferts or @oliverguenther context on this.

[ulferts]

The change should be fine.

It is a bit harder to tell since the PR does not provide any specs which makes understanding the purpose of the change harder.

The move to using the model's error object actually is a fairly new development that was made necessary by shifting to Primer forms. They rely on the errors being in the AR errors object. While it would have been possible to have a decorator or some other mechanism to wrap the model and the error into a single object to work with Primer forms, that would have been necessary to be done in all the places that use Primer forms which in the long run should be a large portion of the application.

[NobodysNightmare]

The comment about missing specs is fair. I am going to address that, because the implementation actually caused some headaches for me.