search

LM-Commons / LmcUser

A generic user registration and authentication module for Laminas. Supports Laminas\Db and Doctrine2. (Formerly ZfcUser)
BSD 3-Clause "New" or "Revised" License
15 stars 16 forks source link

Contribution #4

Closed alexz707 closed 5 months ago

alexz707 commented 3 years ago

Hi! I have found your "organisation" while I was rebuilding my own version of ZfcUser. As you have already some other projects converted to Laminas and because I'm not a fan of several user modules which are only maintained by the creators themself I have deleted my "organisation" and if you want I can contribute to yours. You can have a look at https://github.com/LMTemp/LMFriends-mvc-user there is everything I have changed to the ZfcUser.

Basically there are a few things which would be good to integrate into this project which I already have in my fork:

I would also suggest to add the ZfcUserDoctrineORM to this organisation - I have used it a lot and would like (need) to port it to laminas

What do you think about it? Regards Alex

visto9259 commented 3 years ago

@alexz707, The intent of LM-Commons is for the community to help maintain these libraries. Your help is welcomed. @matwright has been maintaining LmcUser and I suggest that you create one or more pull requests so we can look at it.

As far as ZfcUserDoctrineORM, I believe you are not the first one to mention that it should be ported to Laminas as well. If you are willing to take that work, we can import the repo from ZF-Commons and make you one of the maintainers.

alexz707 commented 3 years ago

Hi! ok, so I will try to break the changes into small pieces so it's easier to review them. Some questions before I start:

I'm used to have the factories next to the classes - would you mind if I would change that? Can we changed the PSR-0 to PSR-4 ? PSR-0 is deprecated and PSR-4 would allow to get rid off the LmcUser Folder under the src dir.

Yes sure I'm willing to take the work! I need it for some projects and I it's better to have one place where the packages are maintained instead of several "own" projects which are not really maintained.

Regards Alex

visto9259 commented 3 years ago

@alexz707, Just to make sure, you want to take on the work of migrating ZfcUserDoctrineORM to Laminas? If so, I will create the repo under LM-Commons for it and give you the rights to the repo. Let's call it LmcUserDoctrineORM.

visto9259 commented 3 years ago

@alexz707, On the other topics, we have to keep in mind that we have many people using this library before we make changes to it.

On the factories issues, I also like to keep factories in the same folder as the class. At this point, there is no added value to changing that. I would wait for a major release to make that change as it introduces breaking change in namespaces forcing everyone to go through their code. Honestly, I don't know when or what would trigger a major release.

On the PSR-0 issue, I agree that we should upgrade to PSR-4. @matwright, Any thoughts?

alexz707 commented 3 years ago

@alexz707, Just to make sure, you want to take on the work of migrating ZfcUserDoctrineORM to Laminas? If so, I will create the repo under LM-Commons for it and give you the rights to the repo. Let's call it LmcUserDoctrineORM.

Yes I want to do that migration :)

@alexz707, On the other topics, we have to keep in mind that we have many people using this library before we make changes to it.

On the factories issues, I also like to keep factories in the same folder as the class. At this point, there is no added value to changing that. I would wait for a major release to make that change as it introduces breaking change in namespaces forcing everyone to go through their code. Honestly, I don't know when or what would trigger a major release.

On the PSR-0 issue, I agree that we should upgrade to PSR-4. @matwright, Any thoughts?

You are right. Is this module already in use by many people? When I look at the other homegrown forks of it I would say there is a lot of work to do to make it "famous" again ;) Of course their shouldn't be breaking changes in minor releases... but is moving a factory a breaking change? Because the module config would also implement that change so the change wouldn't be recognized. But that's only cosmetical - there is no improvement except readability.

Regards Alex

visto9259 commented 3 years ago

@alexz707, Just to make sure, you want to take on the work of migrating ZfcUserDoctrineORM to Laminas? If so, I will create the repo under LM-Commons for it and give you the rights to the repo. Let's call it LmcUserDoctrineORM.

Yes I want to do that migration :)

I created the repo for LmcUserDoctrineORM with an import of ZfcUserDoctrineORM so that is has the entire history, branches, etc. and assigned you to the Dev team for it.

I have no set opinion on whether to keep all the tags and branches since I do not use it.

We have not set a clear scheme on how to release the library once it is migrated. I did a major release for LmcRbacMVC. Others did a minor release. The important thing is to document the BC in namespaces and any configuration. For config, we use lmc_xxx as a key where xxx is the library name like lmc_user or lmc_rbac.

I would suggest to add test scripts if you can and some CI like Travis as well to CI-test any future pull requests.

When you are ready to publish to Packagist, let me know as I will have to create the package the first time and make you a maintainer.

visto9259 commented 3 years ago

Of course their shouldn't be breaking changes in minor releases... but is moving a factory a breaking change? Because the module config would also implement that change so the change wouldn't be recognized. But that's only cosmetical - there is no improvement except readability.

Refactoring a factory is a BC to me as I have no idea how people use the factories. I would not assume that everyone uses the Service Manager to create services. Probably not a good practice to call factories directly but then again, I can see reasons why to do it like create their own private instance of a class or service. In that case, their code would break.

So I would leave factories where they are for now.

alexz707 commented 3 years ago

@alexz707, Just to make sure, you want to take on the work of migrating ZfcUserDoctrineORM to Laminas? If so, I will create the repo under LM-Commons for it and give you the rights to the repo. Let's call it LmcUserDoctrineORM.

Yes I want to do that migration :)

I created the repo for LmcUserDoctrineORM with an import of ZfcUserDoctrineORM so that is has the entire history, branches, etc. and assigned you to the Dev team for it.

I have no set opinion on whether to keep all the tags and branches since I do not use it.

We have not set a clear scheme on how to release the library once it is migrated. I did a major release for LmcRbacMVC. Others did a minor release. The important thing is to document the BC in namespaces and any configuration. For config, we use lmc_xxx as a key where xxx is the library name like lmc_user or lmc_rbac.

I would suggest to add test scripts if you can and some CI like Travis as well to CI-test any future pull requests.

When you are ready to publish to Packagist, let me know as I will have to create the package the first time and make you a maintainer.

Perfect, thank you I will inform you when I'm ready. As there are so many changes I think if someone wants to use the "old" tags and versions, they will stick to the old repos with the old name. I guess the old tags/history doesn't make sense...

I'll have a look at the LmcRbacMVC and will document the BCs.

I have worked with Travis and so on a while ago but of course I'll implement the tests :)

matwright commented 3 years ago

@alexz707, On the other topics, we have to keep in mind that we have many people using this library before we make changes to it.

On the factories issues, I also like to keep factories in the same folder as the class. At this point, there is no added value to changing that. I would wait for a major release to make that change as it introduces breaking change in namespaces forcing everyone to go through their code. Honestly, I don't know when or what would trigger a major release.

On the PSR-0 issue, I agree that we should upgrade to PSR-4. @matwright, Any thoughts?

Yes, I think that's a good idea. I'll can set up a new version dev branch for PSR-4 work and we can keep current version dev for any patches BC breaking updates. Would that work for you?

alexz707 commented 3 years ago

@visto9259 could you please set the default branch to master for the LmcUserDoctrineORM? I have already pushed the changes to the master but can't set the default branch. Thanks :)

visto9259 commented 3 years ago

@alexz707 Done.

alexz707 commented 3 years ago

@alexz707 Done.

Thanks - I think the Repos is ready for Packagist. :) Have a look at it, I used the Laminas Coding Standard and travis/coveralls. It's also already tagged but marked as pre release.

alexz707 commented 3 years ago

@alexz707, On the other topics, we have to keep in mind that we have many people using this library before we make changes to it. On the factories issues, I also like to keep factories in the same folder as the class. At this point, there is no added value to changing that. I would wait for a major release to make that change as it introduces breaking change in namespaces forcing everyone to go through their code. Honestly, I don't know when or what would trigger a major release. On the PSR-0 issue, I agree that we should upgrade to PSR-4. @matwright, Any thoughts?

Yes, I think that's a good idea. I'll can set up a new version dev branch for PSR-4 work and we can keep current version dev for any patches BC breaking updates. Would that work for you?

@matwright that's a good idea. The PSR-4 change shouldn't break anything because it's just behind the namespaces... If you are fine with it, I could just make a branch at my fork and push it when ready?

visto9259 commented 3 years ago

@alexz707 Packagist is set. Added you as a maintainer. Would not be a bad idea to remove older tags.

alexz707 commented 3 years ago

@alexz707 Packagist is set. Added you as a maintainer. Would not be a bad idea to remove older tags.

@visto9259 Thanks a lot, already deleted the tags.

matwright commented 3 years ago

The latest PR's have been reviewed and merged into Master branch. A pre-release v3.4.0 is ready for further testing. This contains the module refactoring, php8 compat. and the Captcha functionality.