remove calls to drupal_static()

My experience is from symfony, rather than d7 - so I could be missing something.

My perspective is that in general we have two options - use the Config or State system.

as for refactoring the FlagCountManager there is only one viable choice - we don't need to deploy our counts from development to production.

so the state system it is.

If there is some hidden aspect of drupal I am missing please let me know ... I will have some time tomorrow morning to make a start on this.

state is something else, that's for persisting. drupal_static() isn't about persisting any values, just store them for the current request.

If you have a service, you just replace drupal_static() with $this->something.

I don't think there is anything official. reset() is a pretty common method name for it, or resetSomething().

In some cases, it might simply no longer be necessary, but I guess that's not the case for flag. There are two reasons for drupal_static(). not just being able to reset it yourself, but also to be able to "clean" the static caches during test runs, which needs to switch the entire environment. This now works automatically because we have a different service container. But that's usually only relevant for low-level stuff.

Either way, the goal is to avoid calling those methods as much as possible. They're usually only relevant in web tests, where there test runner doesn't know what web requests are doing. Most other cases should try to invalidate that kind of information automatically, e.g. changing a flag for a user should update the count/is flagged information.

Postponing until the core issue is resolved.

Currently, we only use drupal_static() in FlagCountManager. It occurs to me that if we replace the SQL backed counts with a cache system, would we even need drupal_static()?

The cache itself doesn't have a static cache, so if you expect multiple calls for the same information in the same request, you want to keep the static cache.

Core often uses reset() or reset*() as method names I'd say. But in almost all cases, you only need those kind of resets in web tests, where you want to assert updated counts/stuff after web requests that changed it in a different request.

For the same request, you want to make your services able to handle changes automatically. You already have events for flagging/unflagging and implement them in FlagCountManager. Just check if you already have counts loaded for the affected static caches (or properties) and if so, either update them automatically (then you save another query if it happens in the same request) or just reset them if a flag happens.

You currently on call drupal_static_reset() in FlagCountsTest which is an API test (and could be converted into a 10x faster kernel test). If you implement the above suggestion, that will just work and you don't need to do anything anymore. And if someone really needs to reset that information, then they can just reset the service with $container->set('ID', NULL). The next call will re-create a new object instance and will load the data again.

This is what I meant.

FlagCountsTest is passing with this for me.

I noticed that two of the queries are actually directly against the flagging table. Technically, we should change them to aggregate entity queries.

I have taken a good look at this patch .. and I agree it is the right direction to take. So +1 from me.

The changes to FlagCountManager manager look good.

Technically, we should change them to aggregate entity queries

Yep I thought that also ....

Here is a problem that has been on my mind for a while.... We have no way of performance checking this patch, or any patch for that matter.

Or answering more complex questions like .. If I have a site with a large number of flags how will change A change performance.

I think a scenario test is often the only way. What I mean is way of generating lots of traffic/activity at random on a continuous basis and running integrity checks.

The issue has forced me to publish something I have been working on ... it is not yet complete but I hope the projects page ( README.md) defines what I see is as the problem and solution.

https://github.com/martinfrances107/flag_line

In short one command line process generates flags at random which acts as a ticket - it gets stamped on entering the rail system. Another command line process unflags/collects tickets as the passenger leaves. The load can be scaled.

At no point does the scenario test use the user interface - it is a performance check for the FlagService/FlagCountManager only.

Anyway I must pull my finger out and finish this ... back to the patch

I think the flag module is in a early enough development stage for this to go it without concern for how this may affect performance.

I think this issue itself doesn't have a performance impact unless you specifically flag *and* get the counts repeatedly inside the same request, which I is probably very uncommon (flagging many times could happen, but not also getting the counts, I think?).

conversion to entity queries could have a certain impact on performance.

I like the direction this patch is taking, it makes more sense than the old approach. Everything passes locally too. Is there anything else that this patch needs?

Technically, we should change them to aggregate entity queries

Let's split that off into a separate issue.

Committed. Thanks!