the policy name in config forNoLimiter isno_limit - it's nice to be consistent for discoverability purposes (personally, I like thenoop name better)

Hmm, yeah I really likenoop too - especially since the other methods don't include "Limit" in their name.

Let's see what others think.

this is a bit a nitpick... but I like type correctness/safety where possible

this function is intended to create aslidingWindow or its specialization (yes the limiter classes are final so specialization is out-of-reach; but his might change in the future)

generalization here potentially allowsslidingWindow to return any other Limiter policy in otherRateLimiterBuilder specializations (consider marking this class asfinal) which is undesirable/unexpected... and might require upstream casting of return value if the concrete implementation method/property is required (currently not the case - all Limiter implementation have the same interface)

Using the implementation class name in the return type prevents us from doing any refactoring in the future (like returning a decorated implementation for instance) as widening a return type is a BC break.
For instance, we could imagine that in the future the returned limiters could be decorated with a traceable limiter in dev to be integrated with the profiler.

And having the same interface for all limiter implementations is the whole point of having a LimiterInterface as the main type in the component.

TL;DR
I'd suggest making thisRateLimiterBuilder classfinal

Using the implementation class name in the return type prevents us from doing any refactoring in the future (like returning a decorated implementation for instance) as widening a return type is a BC break.

I see it as a way to prevent an unsafe change; when the type changes this communicates to upstream devs that they need to update their code.
Yes, changing this type is an explicit BC break, and that is actually good - it can be detected with static analyzers and in runtime type checking; using the interface type allows contributions that potentially introduce unexpected behavior in the runtime. Interface approach leaves the function name as the sole 'contract' - more of a phpDoc hint.

For instance, we could imagine that in the future the returned limiters could be decorated with a traceable limiter in dev to be integrated with the profiler.

This is still possible to do with expected implementation classes, but requires more work. It would require proxy class code generation (like Doctrine does, complicated), or creating tracing specializations of each limiter policy (does not scale well), or use of some AOP library (slow)...

And having the same interface for all limiter implementations is the whole point of having a LimiterInterface as the main type in the component.

Interface is preferably ok for

The contract for the methods of this class is type checked within this test

This would go against the IoC principle, which we strive for. Symfony is a piece of reusable and extensible software, it has to provide flexibility and face the possibility that people will do silly things.

Closing the implementation like you're suggesting is on the contrary building on the idea that we should prevent end users from doing those silly things, at the cost of also preventing power users from doing what they want to achieve.

That's not the approach we have in this project as a whole. We always favor SOLID principles because they're empowering end users (nothing prevents silly things anyway.)

Yes, changing this type is an explicit BC break, and that is actually good

No it is not good, because it means it is actually forbidden to change this (due to our BC policy) and so we would not be able to add support for traceable limiters in the future (duplicating traceable logic to have implementations done using inheritance instead of composition is a no-go. We did things like that in the early days of Symfony and it was a maintenance nightmare)

with all due respect

This would go against the IoC principle, which we strive for. ...

IoC, here, is possiblebecause this class does not followSOLID principles:
S - breached - multiple classes instantiated
O[C] - not enforced - not open for extension (not implemented, do not confuse withclass extends), not closed (this isclass extends, and it is not forbidden)
L - not enforced - interface for return type makes breaching this one easy
I - not implemented
D - ok - made possible by compromises above

Closing the implementation like you're suggesting is ...

... in the favor ofC inO[C] principle ofSOLID

That's not the approach we have in this project as a whole. We always favor SOLID principles because they're empowering end users (nothing prevents silly things anyway.)

I agree,SOLID is a great design guide to follow (albeit class explosion and discovery), but this is a utility class and should probably be spared from'power'-future-proofing.

I strongly believe that the architecture should favorboth empowerment of users and prevention of invalid state (a.k.a. foot-guns or even CVEs)

(duplicating traceable logic to have implementations done using inheritance instead of composition is a no-go. We did things like that in the early days of Symfony and it was a maintenance nightmare)

This, however, is quite SOLID argument ;)