Skip to content

Conversation

@loco8878
Copy link

No description provided.

@loco8878
Copy link
Author

loco8878 commented Sep 16, 2024

Hi guys, can you do a review here?

The pull request is for this ticket

@florianeckerstorfer florianeckerstorfer changed the title Pr/261 Migrate Zend Framework integration to Laminas Sep 16, 2024
Copy link
Member

@florianeckerstorfer florianeckerstorfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to me (although I haven't used Laminas, I have to trust that the tests do their job ;) ) Just one small missed ZF2 → Laminas change and please remove the ZF2 section from the readme.

@florianeckerstorfer
Copy link
Member

@loco8878 And thank you very much for the PR 🎉

@froschdesign
Copy link

Unfortunately, the implementation is not correct. Some things have been forgotten and some are too much. The packages for the module manager and the service manager, for example, are not required.

@froschdesign
Copy link

I can do a review, but unfortunately not immediately.

@froschdesign
Copy link

I suggest to extend the Composer configuration and add the module and config provider because this allows the usage of laminas-component-installer. Which is a Composer plugin for injecting modules and configuration providers into application configuration for laminas-mvc based applications and Mezzio applications.

@froschdesign
Copy link

Here is another suggestion: #261 (comment)

@loco8878
Copy link
Author

Here is another suggestion: #261 (comment)

Input filter is added

@florianeckerstorfer
Copy link
Member

@froschdesign Could you take a quick look at the updated PR and check if it looks good now? Thank you so much in advance

@loco8878
Copy link
Author

@froschdesign Can you please review this PR again? Many thanks.

@loco8878 loco8878 requested a review from froschdesign November 7, 2024 12:52
@loco8878
Copy link
Author

@froschdesign Can you please review this PR again? Many thanks.

@florianeckerstorfer
Copy link
Member

It's been a while, @loco8878 do you think I can go ahead and merge it?

@loco8878
Copy link
Author

@florianeckerstorfer Fine by me. @froschdesign ?

@froschdesign
Copy link

@loco8878
Unfortunately, I do not understand why the ‘Service’ class exists at all. I therefore propose the following changes:

  • remove the SlugifyService (maybe a BC break!)
  • allow the configuration of the filter via config:
    • create a factory
    • check for explicit filter options
    • check for general cocur_slugify options
    • otherwise create the filter without options (the filter already has standard options)
  • allow the configuration of the view helper via config:
    • create a factory
    • check for explicit view helper options
    • check for general cocur_slugify options
    • otherwise create the view helper without options (the view helper should have standard options)

Please also note that there is a new major version of laminas-filter: https://docs.laminas.dev/laminas-filter/v3/migration/v2-to-v3/

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants