I try to decorate some services, that is why I need, that the interface is used instead of the class.

CommentFileSizeAuthor
#20 3432628-20.diff30.57 KBmhh
#12 3432628-12.diff24.74 KBdrupatz
#11 3432628-11.diff32.37 KBdrupatz

Issue fork simple_sitemap-3432628

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Antonín Slejška created an issue. See original summary.

Antonín Slejška opened merge request !89

antonín slejška’s picture

antonín slejška commented
Status: Active » Needs review
hydra’s picture

hydra commented
Status: Needs review » Reviewed & tested by the community

This looks reasonable to me.

walkingdexter’s picture

walkingdexter commented
Status: Reviewed & tested by the community » Needs work

First of all, PHPStan errors need to be fixed https://git.drupalcode.org/issue/simple_sitemap-3432628/-/jobs/1121610.

antonín slejška’s picture

antonín slejška commented
Status: Needs work » Needs review

All tests are green now: https://git.drupalcode.org/issue/simple_sitemap-3432628/-/pipelines/126286

gbyte’s picture

gbyte
Location Berlin
commented
Title: Use SitemapGetterInterface instead of Generator in services » Create and use interface for Generator service
Category: Feature request » Task
Status: Needs review » Needs work

Thanks for looking into this. The generator should have its own interface 'GeneratorInterface' and additionaly implement the existing 'SitemapGetterInterface'. 'GeneratorInterface' should not extend 'SitemapGetterInterface'.

antonín slejška’s picture

antonín slejška commented
Category: Task » Feature request
Status: Needs work » Needs review

Hi @gbyte, I have implemented it, as you suggested. It works by me locally, but it causes the following issues:
https://git.drupalcode.org/issue/simple_sitemap-3432628/-/jobs/1132670

antonín slejška’s picture

antonín slejška commented
Category: Feature request » Task
Status: Needs review » Needs work
antonín slejška’s picture

antonín slejška commented
Status: Needs work » Needs review

I have added the methods setSitemaps and getSitemaps to the GeneratorInterface. It works: https://git.drupalcode.org/issue/simple_sitemap-3432628/-/pipelines/126371
But I'm not sure, if it is the best solution. Why not to use 'extend'?

drupatz’s picture

drupatz commented
StatusFileSize
new 3432628-11.diff32.37 KB

The patches aren't applicable to version 4.2.1, so here's a rewritten patch that will apply

drupatz’s picture

drupatz commented
StatusFileSize
new 3432628-12.diff24.74 KB

Patch from #11 doesn't apply in 4.2.2, therefore here another rewrite

drupatz opened merge request !115

drupatz opened merge request !116

walkingdexter closed merge request !115

walkingdexter closed merge request !116

mhh made their first commit to this issue’s fork.

mhh opened merge request !126

mhh changed the visibility of the branch 3432628-generator-interface-4.2.2 to hidden.

mhh’s picture

mhh commented
StatusFileSize
new 3432628-20.diff30.57 KB

I've created a new MR with all the changes that work with the current 4.x branch (MR !126). I've also rewritten it as a patch to work with 4.2.2.

jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented
Status: Needs review » Needs work

This changes currently do not work. If I do open the config page at /admin/config/search/simplesitemap, then I do get this error:

PHP message: Uncaught PHP Exception Symfony\Component\DependencyInjection\Exception\AutowiringFailedException: "Cannot autowire service "Drupal\simple_sitemap\Manager\Generator": argument "$generator" of method "Drupal\simple_sitemap\Form\StatusForm::_construct()", you should configure its value explicitly.

Tested with version 4.2.2 of simple sitemap.

jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented
jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented
Status: Needs work » Reviewed & tested by the community

The circumstances are different. The MR is against the 4.x-dev branch. There it works. Against version 4.2.2 it just does not apply. Because there are various changes in the 4.x-dev branch which differ from the 4.2.2 release.

I do recommend to merge the changes in 4.x-dev and then create an new patch release (e.g. 4.2.3).

jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented

I can confirm that the patch from #20 does also work, if applied against version 4.2.2.

walkingdexter’s picture

walkingdexter commented
Assigned: antonín slejška » gbyte
Status: Reviewed & tested by the community » Needs review

@gbyte The changes look good to me, please review.

jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented
Status: Needs review » Reviewed & tested by the community
gbyte’s picture

gbyte
Location Berlin
commented
Assigned: gbyte » Unassigned
Status: Reviewed & tested by the community » Needs work

I'm afraid this needs another pass for 4.x before it is merged. I'm happy to merge it afterwards!

jepster_’s picture

jepster_
Location 🇩🇪Düsseldorf
commented
Status: Needs work » Needs review

  • gbyte committed f108aa36 on 4.x 
    feat: #3432628 Create and use interface for Generator service

By:...
gbyte’s picture

gbyte
Location Berlin
commented
Status: Needs review » Fixed

Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.