Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

After changes in auth modules this plugin doesn't work anymore #39

Open
young-druid opened this issue Feb 10, 2021 · 6 comments
Open

After changes in auth modules this plugin doesn't work anymore #39

young-druid opened this issue Feb 10, 2021 · 6 comments

Comments

@young-druid
Copy link

This commit breaks auth_ldap plugin. I think quite minimal changes are needed in your plugin to work with the latest tt-rss.
Current version of plugin gives this error:

Class Auth_Ldap contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (IAuthModule::hook_auth_user)
@hydrian
Copy link
Owner

hydrian commented Feb 10, 2021

Will look into this.

@marcpaulchand
Copy link
Contributor

marcpaulchand commented Feb 12, 2021

Hello at all.

I think i fixed it.
Here is the patch :
L46

- class Auth_Ldap extends Plugin implements IAuthModule {
+ class Auth_Ldap extends Auth_Base {

L76

-         $this->base = new Auth_Base($this->link);

L378

- return $this->base->auto_create_user($ttrssUsername);
+ return $this->auto_create_user($ttrssUsername);

L381

-                    return $this->base->auto_create_user($login);
+                   return $this->auto_create_user($login);

Is it ok for you ?

#40 has been created in order to fix it.

But if someone still got a previous version, how to manage both versions ?
Like tt-rss is in rolling release, should this repo be always compatible with upstream, and got a branch/tag for previous versions ?

I've been surprised by the auto update which break my installation without notice.

@achanu
Copy link

achanu commented Feb 14, 2021

Tested and approved 👍

Thank you marcpaulchand !

@hydrian
Copy link
Owner

hydrian commented Feb 18, 2021

@marcpaulchand Thanks. I will get it mainlined when I get my dev environment backup.

@gramakri
Copy link

The upstream project has moved to using environment variables instead of config.php based 'define'. Maybe we should consider moving to reading configs from env vars as well?

@hydrian
Copy link
Owner

hydrian commented Apr 14, 2021

@gramakri We should look into this but it doesn't belong on this ticket.

hydrian added a commit that referenced this issue Nov 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants