Follow-on from #2565501: Tests are not working because Dynamic page cache was committed in core.: the lazy builder callback could be moved to the flag service.

CommentFileSizeAuthor
#9 interdiff-7-9.txt1.79 KBmartin107
#9 lazy_builder_service-2568129-9.patch5.21 KBmartin107
#7 lazy_builder_service-2568129-7.patch5.2 KBmartin107
#4 lazy_builder_service-2568129-4.patch5.54 KBgiancarlosotelo
#4 interdiff-2-4.txt5.51 KBgiancarlosotelo
#2 lazy_builder_service-2568129-2.patch3.93 KBgiancarlosotelo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

giancarlosotelo’s picture

giancarlosotelo CreditAttribution: giancarlosotelo at MD Systems GmbH commented
Status: Active » Needs review
FileSize
lazy_builder_service-2568129-2.patch3.93 KB

Moving the lazy builder to a service.

martin107’s picture

martin107 CreditAttribution: martin107 commented

Can I suggest a better home for the function.

Looking at the FlagService methods, each operates directly on a Flag

getFlagByXYX()
getFlaggingXYX()
flag()/unflag()

and I think this new method looks a little out of place.

Would a better location for the lazyLinkBuilder() be as a method on another one of our services.

plugin.manager.flag.linktype
\Drupal\flag\ActionLinkPluginManager

I can see one minor objection ...it might look strange to have to pass in the entity manager, which we would have to do, into something advertised as a PliginManager...

giancarlosotelo’s picture

giancarlosotelo CreditAttribution: giancarlosotelo at MD Systems GmbH commented
FileSize
interdiff-2-4.txt5.51 KB
lazy_builder_service-2568129-4.patch5.54 KB

Ok it seems a bit strange but I also think it is a better place for the lazyLinkBuilder().

joachim’s picture

joachim CreditAttribution: joachim commented

Hmm yeah I'm not convinced of it on the plugin manager either. The plugin manager's job is pretty much just to serve up a plugin when you want one.

I also see @martin107's point that there's a tendency to use FlagService as a dumping ground for everything.

I'll have a ponder...

martin107’s picture

martin107 CreditAttribution: martin107 commented

Giancarlo, I apologies .. now that I look at now that I look at #4.
I think it is better that moving things into FlagService ... but it does look very out of place.

I'll have a ponder...

I personally am not opposed to having a service with only one method in it.

By way of making up for the bad advice .. I will do the work for the next patch .. I will try and get to this tonight.

martin107’s picture

martin107 CreditAttribution: martin107 commented
FileSize
lazy_builder_service-2568129-7.patch5.2 KB

I have moved the function into a LazyLinkBuilder service.

socketwench’s picture

socketwench CreditAttribution: socketwench as a volunteer commented

I'd go for a new service first, and the link plugin manager second. It's not like there isn't precedence in core for this -- CommentDefaultFormatter is a field formatter service also tasked with lazy-building.

"LazyLinkBuilder" service works as a name, although I'd prefer something a little more Flag specific and a little less task specific. "FlagLinkBuilder"?

martin107’s picture

martin107 CreditAttribution: martin107 commented
Title: Move lazy builder callback to the Flag service » Move lazy builder callback into a service.
FileSize
lazy_builder_service-2568129-9.patch5.21 KB
interdiff-7-9.txt1.79 KB

Thanks for the nudge ... FlagLinkBuilder looks better.

giancarlosotelo’s picture

giancarlosotelo CreditAttribution: giancarlosotelo at MD Systems GmbH commented

#6 no worries I am just learning and making patches helps me a lot. So I am happy to collaborate in anyway.

socketwench’s picture

socketwench CreditAttribution: socketwench as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks good to go.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Reviewed & tested by the community » Fixed

Committed, with the class and interface docblock summaries tweaked.

Status: Fixed » Closed (fixed)

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