I've been reviewing on my own the integration of EU Login from PR #58 and would like to share some observations and suggestions. I would also appreciate your thoughts on these points:
The custom abstraction for ticket validation is an excellent approach, as it promotes interoperability and allows users to implement their own logic. This flexibility is a strong asset for this project.
Concerning the TicketValidationInterface, which is intended to be implemented as a service. As such, its constructor should exclude any parameters that are not essential for its construction. This will streamline the service-building process and help maintain a clean interface.
One key aspect that can be improved in TicketValidationInterface is the method signature for validate(RequestInterface $request). Currently, it lacks the string $jobAccount parameter. Adding this parameter directly to the method signature TicketValidationInterface::validate(string $jobAccount, RequestInterface $request);, rather than incorporating it into the constructor, offers several benefits:
Flexibility: By including the jobAccount parameter in the validate method, users can implement the interface without having to modify their existing constructors. This flexibility makes it easier for them to adopt the library in various contexts.
Immutability: Moving the jobAccount parameter from the constructor to the method signature encourages class immutability. Immutable classes are less prone to errors, as their state cannot be changed after instantiation. This stability is particularly valuable for libraries that are intended to be integrated into other services.
Single Responsibility Principle: By separating the jobAccount parameter from the constructor and including it in the validate method, we adhere to the Single Responsibility Principle. This design choice ensures that the constructor focuses solely on building the service, while the validate method takes care of validation logic. This separation of concerns results in a cleaner, more maintainable codebase.
Incorporating these changes into the TicketValidationInterface will not only make the interface more user-friendly but also enhance its overall efficiency and maintainability.
Testing of private methods: I usually recommended to not test private methods directly. Instead of testing private methods directly, I believe we should focus on testing the public API and ensuring that it behaves correctly in various scenarios (through Data Providers). If you feel the need to test a private method, it might be an indication that the method's functionality should be extracted into a separate class with its own public API, which can then be tested independently.
Encapsulation: Private methods are meant to be hidden within a class and are not part of the class's public API. They are implementation details designed to support public methods in achieving their functionality. Testing private methods directly can break encapsulation, as you would be accessing and testing internal logic that should remain hidden from the outside world.
Focus on Behavior: When writing tests, the primary goal is to ensure that the public API behaves as expected. By testing public methods, you are essentially validating that the system's external behavior is correct, regardless of the internal implementation. Directly testing private methods shifts the focus from the overall behavior to the internal implementation, which can lead to fragile tests that break whenever internal logic is changed.
Maintainability: Testing private methods can make code maintenance more challenging. When you test private methods directly, any change in the internal implementation may require updating the tests as well. By focusing on testing public methods, you can refactor or modify the internal implementation without affecting the tests, as long as the public API's behavior remains consistent.
Flexibility: Avoiding tests for private methods allows us to refactor or change the implementation of a class without worrying about breaking tests. This flexibility is crucial for adapting to potential new requirements, optimizing performance, or fixing bugs.
Throwing exceptions instead of returning false offers several advantages for error handling and code readability. In summary, throwing exceptions instead of returning false can result in clearer, more maintainable, and more robust code. It encourages proper error handling, provides valuable debugging information, and allows for a standardized approach to managing errors in your application.
Reusability: I also believe that we should prioritize using existing, reputable libraries instead of rewriting parts of their own library, particularly when it comes to security. Well-established libraries have been extensively tested and refined by a large community of developers, ensuring that they are both reliable and efficient. By leveraging these proven solutions, developers can avoid introducing potential vulnerabilities that can arise from creating custom implementations. Furthermore, established libraries often receive regular updates to address any newly discovered security flaws or to keep pace with evolving industry standards. This ensures that the software remains protected against emerging threats. Relying on tried-and-tested libraries not only saves development time and resources, but also helps maintain a more secure and stable software environment. In this context, we could use existing libraries to validate the request.
Shortly after finishing writing this too long comment, I will create and link a pull request that encompasses the following enhancements:
The introduction of a new TicketValidationInterface concrete implementation, utilizing the ecphp/ecaslibrary. This implementation does not return false but instead throws exceptions when issues arise, requiring users to handle exceptions appropriately. To ensure that developers catch exceptions as needed, we must verify that the calling code is equipped to handle them properly.
Test: The updated tests focus on evaluating the public interface, while private methods are indirectly tested through the expanded PHPUnit data provider list that accommodates various use cases. This approach enables thorough coverage of the codebase while maintaining appropriate encapsulation of private methods.
Types: Uses strict types by default (declare(strict_types=1);). Using strict types can effectively reduce the number of bugs by enforcing type safety, enhancing code readability, self-documenting code, better tooling support and clearer expectations.
Please share your thoughts, as I recognize that you have already invested a significant amount of time and effort into this project.
Hello colleagues,
I've been reviewing on my own the integration of EU Login from PR #58 and would like to share some observations and suggestions. I would also appreciate your thoughts on these points:
TicketValidationInterface, which is intended to be implemented as a service. As such, its constructor should exclude any parameters that are not essential for its construction. This will streamline the service-building process and help maintain a clean interface.TicketValidationInterfaceis the method signature forvalidate(RequestInterface $request). Currently, it lacks thestring $jobAccountparameter. Adding this parameter directly to the method signatureTicketValidationInterface::validate(string $jobAccount, RequestInterface $request);, rather than incorporating it into the constructor, offers several benefits:jobAccountparameter in thevalidatemethod, users can implement the interface without having to modify their existing constructors. This flexibility makes it easier for them to adopt the library in various contexts.jobAccountparameter from the constructor to the method signature encourages class immutability. Immutable classes are less prone to errors, as their state cannot be changed after instantiation. This stability is particularly valuable for libraries that are intended to be integrated into other services.jobAccountparameter from the constructor and including it in the validate method, we adhere to the Single Responsibility Principle. This design choice ensures that the constructor focuses solely on building the service, while the validate method takes care of validation logic. This separation of concerns results in a cleaner, more maintainable codebase. Incorporating these changes into theTicketValidationInterfacewill not only make the interface more user-friendly but also enhance its overall efficiency and maintainability.falseoffers several advantages for error handling and code readability. In summary, throwing exceptions instead of returningfalsecan result in clearer, more maintainable, and more robust code. It encourages proper error handling, provides valuable debugging information, and allows for a standardized approach to managing errors in your application.Shortly after finishing writing this too long comment, I will create and link a pull request that encompasses the following enhancements:
TicketValidationInterfaceconcrete implementation, utilizing theecphp/ecaslibrary. This implementation does not returnfalsebut instead throws exceptions when issues arise, requiring users to handle exceptions appropriately. To ensure that developers catch exceptions as needed, we must verify that the calling code is equipped to handle them properly.data providerlist that accommodates various use cases. This approach enables thorough coverage of the codebase while maintaining appropriate encapsulation of private methods.declare(strict_types=1);). Using strict types can effectively reduce the number of bugs by enforcing type safety, enhancing code readability, self-documenting code, better tooling support and clearer expectations.Please share your thoughts, as I recognize that you have already invested a significant amount of time and effort into this project.
Thanks!