I think @tsteur wrote the Menu class, so I'm not sure if this was his intent or not, but I think the idea for using a variable instead of getModule() directly was so derived classes could override the value. @tsteur can you confirm?

A "find usage" didn't yield any usage outside of this class but maybe @tsteur has more info

It is there for performance. We will do it only once instead of many times

Given it is used only in the URL generator methods I think it's safe to think that there will be no performance regression?

It is called quite often and would be nice to have it just as fast as possible. Especially when it is so easy to achieve. It won't be a huge regression for sure

I think you are confusing "called in total" and "called in each plugin". It is called just a few times in each plugin, and since that method return the plugin name that wouldn't cache the 50 calls, only 1-2-3 calls which is why I'm saying it's useless.

The problem is that we want to leave the constructor free to use for subclasses to use dependency injection. If those classes need to call the parent constructor this is error prone as we can expect beginner plugin developers to miss that (and the errors will be hard to debug). Even for experienced developers this is just a pain for no gain.

It is > 50 times called in total. Whereas now > 100 times in total as called twice in that method. So would save 50 times. Again, it isn't a huge improvement but it was faster this way and plugins could overwrite the value if needed. Also it was nicer to use. If it is not good for DI then I reckon it can be removed.

Again, the total number of calls doesn't matter since the function returns the plugin name. Called once in 50 plugins is not the same as called 50 times in 1 plugin.

Called once in 50 plugins means the cache would not save anything because it would cache a new value every time.

See urlForAction(). It calls $this->getModule() twice in that function. Meaning if it is cached it would be only done once per plugin. When this function urlForAction() is called once per plugin, we save one call to getModule(). If that method is called 2 times per plugin, we save 3 called to getModule() etc. If 50 plugins call urlForAction() once, it saves 50 called in the end since getModule() would be executed 100 times.

Anyway, it is not so important as said if it is not good for DI.

In that case the problem is with urlForAction() where $this->getModule() would simply be put in a variable, there is no need for a cache for getModule(). A quick look in blackfire shows that get_class() doesn't even appear in the profiling, it's no performance concern at all.