Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix LDAP install for systems without ldap extension #846

Merged
merged 7 commits into from
Jun 10, 2019
Merged

Conversation

kevinpapst
Copy link
Member

@kevinpapst kevinpapst commented Jun 10, 2019

Description

Fixes the requirement to the php extension "ldap".
LDAP users now have to execute composer require zendframework/zend-ldap in order to use LDAP authentication.

Updated docs online at: https://www.kimai.org/documentation/ldap.html#installation

See #815

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I verified that my code applies to the guidelines (composer kimai:code-check)
  • I updated the documentation (see here)
  • I agree that this code is used in Kimai and will be published under the MIT license

@kevinpapst kevinpapst added the bug label Jun 10, 2019
@kevinpapst kevinpapst added this to the 1.0 milestone Jun 10, 2019
@kevinpapst kevinpapst changed the title Ldap install fix LDAP install for systems without ldap extension Jun 10, 2019
@kevinpapst kevinpapst mentioned this pull request Jun 10, 2019
8 tasks
@kimai kimai deleted a comment from codecov bot Jun 10, 2019
@kimai kimai deleted a comment from codecov bot Jun 10, 2019
@army1349
Copy link

When I added security section to local.yaml, I got this:

Script cache:clear returned with error code 1
In PrototypedArrayNode.php line 313:
You are not allowed to define new elements for path "security.firewalls". Please define all elements for this path in one config file.

@kevinpapst
Copy link
Member Author

Hm, worked here flawlessly. Ok, I will research whats wrong and report back.

@kevinpapst
Copy link
Member Author

Seems to be a limitation of the Symfony framework.
I could reproduce the problem, when placing the config in configs/packages/local.yaml.

Locally I store my custom config in the environment specific directory, so it doesn't cause problem when running my testsuite. The file configs/packages/local.yaml is always loaded, where the files configs/packages/[dev|prod]/local.yaml are loaded environment specific.

I yet don't understand why this should be any different, but could you please try to move your unchanged config to config/packages/prod/local.yaml and clear the cache again?

@army1349
Copy link

I yet don't understand why this should be any different, but could you please try to move your unchanged config to config/packages/prod/local.yaml and clear the cache again?

Yep, works fine like that.

@kevinpapst
Copy link
Member Author

Smells fishy to manipulate the loading order to get it to work, still searching for the "right way to do it".

This seems to be a longer story, as there is an open issue at Symfony regarding this topic:
symfony/symfony#25021

Will work my way backwards through the Symfony discussions to find a solution...

@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

Merging #846 into master will decrease coverage by 0.1%.
The diff coverage is 77.27%.

@@             Coverage Diff              @@
##             master     #846      +/-   ##
============================================
- Coverage     93.78%   93.68%   -0.11%     
+ Complexity     2338     2332       -6     
============================================
  Files           240      239       -1     
  Lines          7355     7345      -10     
============================================
- Hits           6898     6881      -17     
- Misses          457      464       +7
Impacted Files Coverage Δ Complexity Δ
src/Ldap/LdapAuthenticationProvider.php 94.11% <ø> (-0.62%) 14 <0> (-2)
src/Configuration/LdapConfiguration.php 100% <ø> (ø) 4 <0> (-1) ⬇️
src/Kernel.php 81.25% <100%> (+2.67%) 22 <0> (+1) ⬆️
src/DependencyInjection/Configuration.php 98.68% <100%> (-0.02%) 22 <0> (-1)
src/Ldap/LdapUserProvider.php 84.84% <100%> (-5.16%) 12 <1> (-2)
src/Ldap/LdapDriver.php 81.39% <37.5%> (-13.61%) 12 <6> (+1)

@kevinpapst
Copy link
Member Author

should work now with config/packages/local.yaml as well

@kevinpapst kevinpapst merged commit 833968a into master Jun 10, 2019
@kevinpapst kevinpapst deleted the ldap-install branch June 10, 2019 23:27
@army1349
Copy link

should work now with config/packages/local.yaml as well

Yes, I can confirm, it is working.

@lock
Copy link

lock bot commented Aug 10, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. If you use Kimai on a daily basis, please consider donating to support further development of Kimai.

@lock lock bot locked and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants