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

Add Composer Require Checker configuration #112

Closed
wants to merge 5 commits into from

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Jan 12, 2022

Q A
QA yes

Description

Add composer-require-checker configuration so that CI runs it.

@gsteel gsteel changed the base branch from 3.0.x to 2.19.x January 12, 2022 13:57
@gsteel gsteel changed the title Require checker Add Composer Require Checker configuration Jan 12, 2022
composer-require-checker.json Outdated Show resolved Hide resolved
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Ok, so what do we do about failures now? :D I suppose we replace some class references with strings at first?

+----------------------------------------------------------+--------------------+
| Unknown Symbol                                           | Guessed Dependency |
+----------------------------------------------------------+--------------------+
| Interop\Container\ContainerInterface                     |                    |
| Laminas\Authentication\AuthenticationService             |                    |
| Laminas\Authentication\AuthenticationServiceInterface    |                    |
| Laminas\Escaper\Escaper                                  |                    |
| Laminas\Feed\Writer\Feed                                 |                    |
| Laminas\Feed\Writer\FeedFactory                          |                    |
| Laminas\Filter\FilterChain                               |                    |
| Laminas\Http\Response                                    |                    |
| Laminas\I18n\Translator\TranslatorAwareInterface         |                    |
| Laminas\I18n\Translator\TranslatorInterface              |                    |
| Laminas\Mvc\ModuleRouteListener                          |                    |
| Laminas\Mvc\Plugin\FlashMessenger\FlashMessenger         |                    |
| Laminas\Navigation\AbstractContainer                     |                    |
| Laminas\Navigation\Navigation                            |                    |
| Laminas\Navigation\Page\AbstractPage                     |                    |
| Laminas\Paginator\Paginator                              |                    |
| Laminas\Permissions\Acl\Acl                              |                    |
| Laminas\Permissions\Acl\Role\RoleInterface               |                    |
| Laminas\Router\RouteMatch                                |                    |
| Laminas\Router\RouteStackInterface                       |                    |
| Laminas\ServiceManager\AbstractPluginManager             |                    |
| Laminas\ServiceManager\Exception\InvalidServiceException |                    |
| Laminas\ServiceManager\FactoryInterface                  |                    |
| Laminas\ServiceManager\Factory\InvokableFactory          |                    |
| Laminas\ServiceManager\ServiceLocatorInterface           |                    |
| Laminas\ServiceManager\ServiceManager                    |                    |
| Laminas\Uri\UriFactory                                   |                    |
| Laminas\Validator\Sitemap\Changefreq                     |                    |
| Laminas\Validator\Sitemap\Lastmod                        |                    |
| Laminas\Validator\Sitemap\Loc                            |                    |
| Laminas\Validator\Sitemap\Priority                       |                    |
| Psr\Container\ContainerInterface                         |                    |
| Zend\Authentication\AuthenticationService                |                    |
| Zend\Authentication\AuthenticationServiceInterface       |                    |
| Zend\View\Helper\Asset                                   |                    |
| Zend\View\Helper\BasePath                                |                    |
| Zend\View\Helper\Cycle                                   |                    |
| Zend\View\Helper\DeclareVars                             |                    |
| Zend\View\Helper\Doctype                                 |                    |
| Zend\View\Helper\EscapeCss                               |                    |
| Zend\View\Helper\EscapeHtml                              |                    |
| Zend\View\Helper\EscapeHtmlAttr                          |                    |
| Zend\View\Helper\EscapeJs                                |                    |
| Zend\View\Helper\EscapeUrl                               |                    |
| Zend\View\Helper\FlashMessenger                          |                    |
| Zend\View\Helper\Gravatar                                |                    |
| Zend\View\Helper\HeadLink                                |                    |
| Zend\View\Helper\HeadMeta                                |                    |
| Zend\View\Helper\HeadScript                              |                    |
| Zend\View\Helper\HeadStyle                               |                    |
| Zend\View\Helper\HeadTitle                               |                    |
| Zend\View\Helper\HtmlFlash                               |                    |
| Zend\View\Helper\HtmlList                                |                    |
| Zend\View\Helper\HtmlObject                              |                    |
| Zend\View\Helper\HtmlPage                                |                    |
| Zend\View\Helper\HtmlQuicktime                           |                    |
| Zend\View\Helper\HtmlTag                                 |                    |
| Zend\View\Helper\Identity                                |                    |
| Zend\View\Helper\InlineScript                            |                    |
| Zend\View\Helper\Json                                    |                    |
| Zend\View\Helper\Layout                                  |                    |
| Zend\View\Helper\Navigation\Breadcrumbs                  |                    |
| Zend\View\Helper\Navigation\Links                        |                    |
| Zend\View\Helper\Navigation\Menu                         |                    |
| Zend\View\Helper\Navigation\Sitemap                      |                    |
| Zend\View\Helper\PaginationControl                       |                    |
| Zend\View\Helper\Partial                                 |                    |
| Zend\View\Helper\PartialLoop                             |                    |
| Zend\View\Helper\Placeholder                             |                    |
| Zend\View\Helper\RenderChildModel                        |                    |
| Zend\View\Helper\RenderToPlaceholder                     |                    |
| Zend\View\Helper\ServerUrl                               |                    |
| Zend\View\Helper\Url                                     |                    |
| Zend\View\Helper\ViewModel                               |                    |
+----------------------------------------------------------+--------------------+

@gsteel
Copy link
Member Author

gsteel commented Jan 13, 2022

@Ocramius

  • Container related symbols are solved in Introduces Config Providers #110
  • I think that Laminas\I18n might need to be a dependency
  • FlashMessenger stuff will be removed in 3, so we should probably skip it
  • Laminas\Escaper should definitely be a dependency
  • Also Laminas\Filter - the renderer depends on this for an optional filter chain which is a useful feature
  • Everything Zend related can of course be skipped and then removed in 3 (Edit: Introduces Config Providers #110 Stringifies the Zend class references now)
  • Laminas\Validator is used only for XML sitemap related stuff. I think it should be plausible to inline the validation to avoid the dependency
  • Maybe Laminas\Feed stuff can be dropped - perhaps it could be extracted to a separate package that imports both Feed and View to provide the specific functionality?
  • I'd say the same about Navigation and Acl. Acl is only referenced by Navigation and there's a whole tonne of stuff in there (Laminas\Uri, Validator, Sitemap) that might benefit from being a separate package or moved to Navigation itself.
  • Authentication is used only by the Identity helper - maybe whitelist the symbols and perform some class_exists, throw exceptions stuff there.
  • Laminas\Uri is only referenced by a currently skipped test
  • I reckon that PaginationControl helpers could be moved to the Pagination library??
  • Mvc, Requests, Responses etc is a bigger problem but I also think that moving the related functionality to Mvc is a good solution because Mvc very much depends on view rather than the other way around.

Can we leave this pull open and periodically rebase it? I can list the stuff that we'll definitely skip and then eventually see it go green?

@froschdesign
Copy link
Member

@gsteel
Many things still need to be discussed. For example, if we do not want a hard dependency to laminas-view in laminas-paginator, laminas-navigation or laminas-feed, then moving to these packages is not an option either.

Can we leave this pull open and periodically rebase it?

You can convert the pull request to draft.

@gsteel
Copy link
Member Author

gsteel commented Jan 13, 2022

@froschdesign

Many things still need to be discussed. For example, if we do not want a hard dependency to laminas-view in laminas-paginator, laminas-navigation or laminas-feed, then moving to these packages is not an option either.

Absolutely! However, it is definitely plausible because in these cases, view can be a dev dependency for these packages. i.e. Navigation already makes use of ACL right? and you couldn't use these view helpers without Navigation itself. The helpers wouldn't affect consumers of Nav standalone, but moving them there would mean that users of these helpers would still benefit from the functionality they bring in Mvc for example.

Alternatively, perhaps it's feasible to create new packages that integrate them together in the same way as FlashMessenger or PRG for example, like laminas-view-plugin-navigation

I do understand that this would all require discussion - I feel it's worth bringing it up given the progress towards a 3 release

@froschdesign
Copy link
Member

@gsteel

The helpers wouldn't affect consumers of Nav standalone, but moving them there would mean that users of these helpers would still benefit from the functionality they bring in Mvc for example.

That is exactly what I mean: the current planning for laminas-navigation contains the idea to allow the output with different template engines. Moving the view helper to the laminas-navigation package would therefore only be a temporary solution, which we have to think about. And this is only one example.

But I like the direction for laminas-view! 👍

@gsteel
Copy link
Member Author

gsteel commented Jan 13, 2022

@froschdesign - I know there are larger maintenance issues at stake, but, I've (Quickly) made this: https://github.com/gsteel/laminas-view-plugin-navigation - something like this could become a hard dependency in a 2.x release here and then dropped and suggested in 3.x

This alone would remove a load of dependencies from view.

@froschdesign
Copy link
Member

…I've (Quickly) made this: https://github.com/gsteel/laminas-view-plugin-navigation…

Creating a new repository is not the problem, but is it the right direction for each component or do we want to go down this path? At the moment there are open questions for me and especially from the point of view of the end user.
If we stay with the laminas-navigation example, the question arises why laminas-view is not a dependency of laminas-navigation or does the new component have to be a hard dependency? What does the end user have to do to create and render a navigation? Install several components or only one? How will this look in the future and can this be kept as simple as possible?
Please look at Mezzio and the authentication packages, without reading the code you don't know what needs to be installed. 😞

@gsteel
Copy link
Member Author

gsteel commented Jan 13, 2022

At the moment there are open questions for me and especially from the point of view of the end user.
If we stay with the laminas-navigation example, the question arises why laminas-view is not a dependency of laminas-navigation or does the new component have to be a hard dependency?

Right now, view has a hard-dependency on nav but it's undeclared because the nav helpers are supposed to be optional - there's a suggest and the hope that the end end user can figure out the hard dep on nav. Assuming an integration package is released, and made a hard dep in a view:2 release, the end user would have immediately usable nav helpers because all the deps would be correctly resolved. When view:3 is released, we can document the removal of the nav helpers and point users to the new package; This way, there's a single package to require in a project the makes use of view, in order to do view related stuff with nav containers.

With any luck, navigation itself could drop the dependency on view and then both navigation and view can evolve independently of each other. (Automatic dependency PRs would hopefully surface incompatibilities too)

I do understand the documentation challenges, but also a package to integrate view & nav is the perfect target for this documentation. If something like this is deemed an acceptable course of action, it would make sense to relocate the existing docs for the nav related view helpers to that package too.

@froschdesign
Copy link
Member

@gsteel

When view:3 is released, we can document the removal of the nav helpers and point users to the new package; This way, there's a single package to require in a project the makes use of view, in order to do view related stuff with nav containers.

And here is the problem: the handling is too complicated when I first have to read the documentation to install and run something.

@gsteel
Copy link
Member Author

gsteel commented Jan 13, 2022

@froschdesign

And here is the problem: the handling is too complicated when I first have to read the documentation to install and run something.

I guess I don't fully appreciate this argument. Users of view:2 will just carry on as normal. When they attempt to upgrade to view:3, the breaking changes, changelog and existing docs should be known to them, therefore it's a chore, but BC breaks are permitted in a major and they should likely know where to look or ask.

New-comers to view diving in straight at 2 or 3 are going to have to read some docs to do anything, leave alone get their nav trees all setup and rendering menus and breadcrumbs etc.

In both cases, the answer to "I want to render navigation trees" is effectively "Install this package and read these docs".

I'd also argue that for users already familiar with both view and navigation, extracting it all into a separate package is a relatively trivial change and there's precedent with flash messenger for this.

@froschdesign
Copy link
Member

@gsteel

I guess I don't fully appreciate this argument.

I mean the installation of a component and when I tried to run something and another component is needed. Please see the problems with laminas-form and laminas-paginator which needs laminas-view to render an output or laminas-feed which needs laminas-http to import feeds or Mezzio and the authentication adapters – always the same problem: trial and error. And if the dependencies are not clearly defined or no solution is found that communicates a clear next step of what needs to be installed additionally, then we repeat the old problems.

To be clear, I am not against creating new components or moving the code into the related components. However, it is important to avoid expanding the patchwork and repeat old problems. Therefore, individual decisions are needed that will still fit tomorrow.

@gsteel
Copy link
Member Author

gsteel commented Feb 1, 2022

@Ocramius - OK if I rebase on 3.x for this one? I can't see a way of getting this into 2.x without loads of mucking about

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2022

Yes, let's do it on 3.0.x only

@Ocramius Ocramius modified the milestones: 2.20.0, 3.0.0 Feb 1, 2022
@Ocramius Ocramius changed the base branch from 2.20.x to 3.0.x February 1, 2022 16:23
Signed-off-by: George Steel <george@net-glue.co.uk>
gsteel added 4 commits March 23, 2022 20:51
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel
Copy link
Member Author

gsteel commented Mar 23, 2022

The list is getting shorter…

+-------------------------------------------------------+--------------------+
| Unknown Symbol                                        | Guessed Dependency |
+-------------------------------------------------------+--------------------+
| Laminas\Authentication\AuthenticationService          |                    |
| Laminas\Authentication\AuthenticationServiceInterface |                    |
| Laminas\Feed\Writer\Feed                              |                    |
| Laminas\Feed\Writer\FeedFactory                       |                    |
| Laminas\Filter\FilterChain                            |                    |
| Laminas\Http\Response                                 |                    |
| Laminas\I18n\Translator\TranslatorAwareInterface      |                    |
| Laminas\I18n\Translator\TranslatorInterface           |                    |
| Laminas\Mvc\ModuleRouteListener                       |                    |
| Laminas\Mvc\Plugin\FlashMessenger\FlashMessenger      |                    |
| Laminas\Navigation\AbstractContainer                  |                    |
| Laminas\Navigation\Navigation                         |                    |
| Laminas\Navigation\Page\AbstractPage                  |                    |
| Laminas\Paginator\Paginator                           |                    |
| Laminas\Permissions\Acl\Acl                           |                    |
| Laminas\Permissions\Acl\Role\RoleInterface            |                    |
| Laminas\Router\RouteMatch                             |                    |
| Laminas\Router\RouteStackInterface                    |                    |
| Laminas\Uri\UriFactory                                |                    |
| Laminas\Validator\Sitemap\Changefreq                  |                    |
| Laminas\Validator\Sitemap\Lastmod                     |                    |
| Laminas\Validator\Sitemap\Loc                         |                    |
| Laminas\Validator\Sitemap\Priority                    |                    |
+-------------------------------------------------------+--------------------+

@gsteel gsteel removed this from the 3.0.0 milestone Oct 11, 2022
@gsteel gsteel added Won't Fix This will not be worked on and removed Enhancement labels Oct 11, 2022
@gsteel
Copy link
Member Author

gsteel commented Oct 11, 2022

Closing, pending resolution of laminas/laminas-ci-matrix-action#131

This PR will be a pig to rebase and merge and it'll be a long time before CI passes require-checker.

@gsteel gsteel closed this Oct 11, 2022
@gsteel gsteel deleted the require-checker branch October 11, 2022 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Won't Fix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants