Skip to content

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Jan 18, 2023

Summary of Changes

Introduces an Ldap factory in the authentication plugin for better testing capability. The existing test is moved to the integration test suite as it is not a unit test. A new unit test class is added as well and some smaller code cleanup like camel case variables.

This should only be considered after #39624 got merged.

Testing Instructions

Is covered by unit and integration test.

Actual result BEFORE applying this Pull Request

All works.

Expected result AFTER applying this Pull Request

All works.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@laoneo laoneo marked this pull request as ready for review January 20, 2023 07:01
@tatankat
Copy link
Contributor

@laoneo Could you include the configuration and documentation changes in #38775 ? The ldap tests are also moved to Integration there.

@laoneo
Copy link
Member Author

laoneo commented Jan 23, 2023

Was not aware of #38775. This one should be merged first, and then I will adapt mine as well.

@laoneo
Copy link
Member Author

laoneo commented Jan 26, 2023

@tatankat can you have a look here as I merged your changes from #38775.

@tatankat
Copy link
Contributor

The changes already handle some of my TODOs after my PRs are accepted, so thanks for that :)

But when I try a login, I get:
Joomla\Plugin\Authentication\Ldap\Extension\Ldap::__construct(): Argument #1 ($factory) must be of type Joomla\Plugin\Authentication\Ldap\Factory\LdapFactoryInterface, Joomla\Event\Dispatcher given, called in /var/www/html/timo/joomla-cms-composed/plugins/authentication/ldap/services/provider.php on line 42
The tests succeed however. Can the register function of the provider be tested?

@laoneo
Copy link
Member Author

laoneo commented Jan 27, 2023

Somehow the changes got reverted in the provider file. Fixed that.

@tatankat
Copy link
Contributor

It's working now

@obuisard obuisard added this to the Joomla! 4.3.0 milestone Jan 28, 2023
@obuisard obuisard merged commit 05210a3 into joomla:4.3-dev Jan 28, 2023
@obuisard
Copy link
Contributor

Thank you Allon @laoneo!

@laoneo laoneo deleted the authentication/ldap/factory branch January 28, 2023 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants