What do you mean by "it breaks them" ? This private method is in a new class, not in an existing class.

Well if your application usesMessageDigestPasswordEncoder then an overwrittenmergePasswordAndSalt method will never be used, as the code was copied toMessageDigestPasswordHasher andMessageDigestPasswordEncoder now usesMessageDigestPasswordHasher viaLegacyEncoderTrait

So then the next step is, like the deprecation message suggest, to useMessageDigestPasswordHasher directly.

But asmergePasswordAndSalt is private, mergePasswordAndSalt cannot also not be overwritten.

Both cases break current apps that need to overwrite mergePasswordAndSalt.

If MessageDigestPasswordEncoder broke its extension point, then this is indeed a bug that should be fixed.

However, I'm not sure we should makemergePasswordAndSalt an extension point in the new component. I'd rather tagMessageDigestPasswordHasher as@final. If a project wants to roll its own crypto, making it implement the full interface seems better to me (as that should not be encouraged anyway)

I am not sure what the advantage of using private methods or@final is?

All it does is to force me to duplicate code. Making the methodprotected solves this and brings no disadvantages that I can see.

Making the method protected has a maintenance cost, as it forces us to preserve BC on it, forbidding us to refactor things in some ways (for instance, here, we will have to revert the usage of the LegacyEncoderTrait in that class).
And inheritance-based extension points are actually the more costly ones in term of backward compatibility maintenance.

I see. Well I tried 😉

Should I close this PR?

As explained by@stof, this PR is not a bugfix. And I agree with his suggestion to implement the new interface directly.

@derrabus we should still fix the regression in MessageDigestPasswordEncoder which was refactored to use the LegacyEncoderTrait without accounting for the fact that it was extendable with a protected method though.

@stof#41703