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

remove laminas/laminas-loader #88

Open
arhimede opened this issue Nov 27, 2024 · 14 comments
Open

remove laminas/laminas-loader #88

arhimede opened this issue Nov 27, 2024 · 14 comments
Assignees

Comments

@arhimede
Copy link
Member

Feature Request

Q A
New Feature yes
RFC yes
BC Break yes

Summary

Since laminas/laminas-loader is abandoned, we should remove it from composer.json

@laminas-bot
Copy link
Contributor

This package is considered feature-complete, and is now in security-only maintenance mode, following a decision by the Technical Steering Committee.
If you have a security issue, please follow our security reporting guidelines.
If you wish to take on the role of maintainer, please nominate yourself

You can continue using laminas/laminas-http safely.
Its successor will be PSR-7 in a later revision of laminas/laminas-mvc.

@arhimede
Copy link
Member Author

yeah, it is not necessary a new release , the issue will be fixed downstream

@arhimede arhimede reopened this Nov 27, 2024
@froschdesign
Copy link
Member

@arhimede

yeah, it is not necessary a new release , the issue will be fixed downstream

laminas-loader must be removed but this needs also a replacement for HeaderLoader:

namespace Laminas\Http;
use Laminas\Loader\PluginClassLoader;
/**
* Plugin Class Loader implementation for HTTP headers
*/
class HeaderLoader extends PluginClassLoader

@arhimede
Copy link
Member Author

@froschdesign then what should we do here, since this package is in security-only mode ?
Should we have a new release of it, still ?

@froschdesign
Copy link
Member

@arhimede

…since this package is in security-only mode ?

Is not! See your own polls. 😄

@gsteel
Copy link
Member

gsteel commented Nov 27, 2024

We voted not to abandon this one. It's integral to MVC so we can't just kill it off. I don't understand the need to deal with the abandonment of the loader library - instead we should focus on eradicating the use of this library in all dependents so that we can abandon it at a future date… which is gonna take a while…

@arhimede
Copy link
Member Author

arhimede commented Nov 27, 2024

@arhimede

…since this package is in security-only mode ?

Is not! See your own polls. 😄

I saw that , but Laminas bot closed automatically this issue because "security-only"
#88 (comment)
Do we have an issue with laminas-bot ?

@froschdesign
Copy link
Member

Do we have an issue with laminas-bot ?

No, no problem, but as long as the workflow is still active, the message will keep coming back and issues will be closed: https://github.com/laminas/laminas-http/blob/2.21.x/.github/workflows/auto-close.yml

@arhimede
Copy link
Member Author

Do we have an issue with laminas-bot ?

No, no problem, but as long as the workflow is still active, the message will keep coming back and issues will be closed: https://github.com/laminas/laminas-http/blob/2.21.x/.github/workflows/auto-close.yml

There is an active auto-close Action.
https://github.com/laminas/laminas-http/blob/2.21.x/.github/workflows/auto-close.yml
Should we remove it maybe ?

@froschdesign
Copy link
Member

Should we remove it maybe ?

Yes, it needs to be removed.

@arhimede arhimede reopened this Nov 27, 2024
@arhimede
Copy link
Member Author

ok, @alexmerlin Let's see what we can do here, i re-opened the ticket

@alexmerlin
Copy link
Member

I analyzed the code and found that there are 2 places where laminas-loader is being used:

  • src/HeaderLoader.php - extends Laminas\Loader\PluginClassLoader which is an implementation of the Laminas\Loader\PluginClassLocator interface
  • src/Headers.php - contains $pluginClassLoader which is of type Laminas\Loader\PluginClassLocator

The simplest solution to allow the removal of laminas-loader would be to copy PluginClassLoader and PluginClassLocator inside of laminas-http.
But, being that Headers->setPluginClassLoader() is called from outside of laminas-http, not sure if the caller could be set up to call setPluginClassLoader with the custom HeaderLoader.

@gsteel What do you think?

@gsteel
Copy link
Member

gsteel commented Nov 28, 2024

Changing the inheritance hierarchy of HeaderLoader will be a BC break, and the reason loader is abandoned is because we have composer now.

Seeing as the inheritance changes would be a BC break, we might as well put the effort in to refactoring HeaderLoader instead so that it doesn't rely on loader at all.

I'm guessing, it turns content-type into Laminas\Http\Header\ContentType or something like that… and allows users to provide custom header implementations for x-my-header to My\Header\ClassName - I would start by re-implementing that feature so that it doesn't require any external dependencies.

Releasing a major just to get rid of loader seems excessive… especially on a lib that is destined to be abandoned itself.

Could we focus effort of ridding other libs of laminas-http instead?

I may be off the mark - it's been a long time since I looked at laminas-http

@froschdesign
Copy link
Member

@gsteel

…especially on a lib that is destined to be abandoned itself.

The vote says something different here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants