laminas / laminas-validator

Validation classes for a wide range of domains, and the ability to chain validators to create complex validation criteria
https://docs.laminas.dev/laminas-validator/
BSD 3-Clause "New" or "Revised" License
126 stars 55 forks source link

[RFC]: Drop Support for MVC in 3.0 so it can be added later without a BC break #336

Closed gsteel closed 4 weeks ago

gsteel commented 1 month ago

RFC

Validator ships a Module class without importing any symbols from MVC.

There are no tests for it and v3.0 of this lib will not work with any MVC project until everything supports ServiceManager v4.x

Proposal

Remove the Module class now, for a 3.0 release so that it can be reimplemented in a minor release, if necessary, in the future.

Reasoning

By the time MVC supports SMv4, who knows what changes will need to occur to the Module class here, at which point we need to break BC on this class just so that MVC can be supported.

CC/ @laminas/technical-steering-committee

boesing commented 1 month ago

I'd just remove the init method and provide a proper factory which extracts validators from merged config. never understood the servicelistener way and it actually shifts that logic to mvc only. might be more useful to deprecate module Manager Interfaces related to this, having a dedicated config should be considered default instead of having random modules implementing these Provider Interfaces.

gsteel commented 1 month ago

Why not just bin the whole Module class and deal with it later. No Mvc project will be able to install 3.0 of validator anyhow 🤷‍♂️

boesing commented 1 month ago

that is correct, but unless it is officially dumped, it feels wrong to drop stuff related to it

froschdesign commented 1 month ago

it feels wrong to drop stuff related to it

I have the same concerns but on the other side…

laminas-mvc in version 4 removes the module manager and if version 3 of laminas-validator does not work with version 3 of laminas-mvc, then I do not know why the module class is still needed. And I assume that version 4 of laminas-mvc is required to support the latest version of the service manager.

Xerkus commented 1 month ago

While mvc won't depend on module manager the module support will be there via a separate alternate glue package.

Module in here does not care about mvc however. It configures service listener that is a part of the module manager. Service listener operates on module load events.

gsteel commented 1 month ago

So, from what I understand there @Xerkus the Module class here will simply not be relevant or compatible with MVC v4? And it's very unlikely that support for validator:3.0 will be implemented in MVC:3.x because MVC:3 won't support ServiceManager:4.0 ... ??

boesing commented 1 month ago

I'd just recommned to remove the modulemanager specific stuff but why not keep the Module class.

If we are talking about that:

It looked to me that MVC won't be developed anymore but if there is active development on MVC v4 (not just ideas but actual active development) then we should actually outline that. My latest understanding was that MVC will be abandoned in favor of mezzio. I'd rather spend time on having all the modulemanager/MVC related stuff working with mezzio (i.e. by providing a component which can be registered into pipelines to have MVC events triggered, etc.) and rip off MVC at all than actually developing a new major for MVC v4 - but might be just my personal PoV.

Xerkus commented 1 month ago

It is orthogonal to mvc. I expect ModuleManager to be updated to SM 4. I would also expect updated module manager to be able deal with config providers directly.

Service listener provides a key in $config and getValidatorConfig() in userland modules as a way to configure validator manager. I don't think anyone sane uses getValidatorConfig(). I would rather kill service listener with fire.

The problem then really is in here https://github.com/laminas/laminas-validator/blob/d00ffe34f2de77d14c555b0cdc7b7ebccc83ddc5/src/ValidatorPluginManagerFactory.php#L23-L26

froschdesign commented 1 month ago

I would rather kill service listener with fire.

👍🏻

The problem then really is in here

Then it will be removed with version 3 of this package.