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

Comments

joachim created an issue. See original summary.

giancarlosotelo’s picture

giancarlosotelo commented
Status: Active » Needs review
StatusFileSize
new lazy_builder_service-2568129-2.patch3.93 KB

Moving the lazy builder to a service.

martin107’s picture

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 commented
StatusFileSize
new interdiff-2-4.txt5.51 KB
new 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 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 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 commented
StatusFileSize
new lazy_builder_service-2568129-7.patch5.2 KB

I have moved the function into a LazyLinkBuilder service.

socketwench’s picture

socketwench 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 commented
Title: Move lazy builder callback to the Flag service » Move lazy builder callback into a service.
StatusFileSize
new lazy_builder_service-2568129-9.patch5.21 KB
new interdiff-7-9.txt1.79 KB

Thanks for the nudge ... FlagLinkBuilder looks better.

giancarlosotelo’s picture

giancarlosotelo 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 commented
Status: Needs review » Reviewed & tested by the community

Looks good to go.

joachim’s picture

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.