verbb / navigation

A Craft CMS plugin to create navigation menus for your site.
Other
90 stars 23 forks source link

Avoid unnecessary queries #268

Closed VesninAndrey closed 3 years ago

VesninAndrey commented 3 years ago

I've found 114 similar queries just for "plugins.navigation.schemaVersion".

Why we need a db query here?

https://github.com/verbb/navigation/blob/de785905f707cca5e30cd67e449a2e9085d2b150/src/elements/db/NodeQuery.php#L150

        // Use a database call for performance
        $schemaVersion = (new Query())
            ->select(['value'])
            ->from('{{%projectconfig}}')
            ->where(['path' => 'plugins.navigation.schemaVersion'])
            ->scalar();

        $schemaVersion = Json::decodeIfJson($schemaVersion);

Supposed to be something like

$schemaVersion = Navigation::getInstance()->schemaVersion;

engram-design commented 3 years ago

Refer to my comments in this issue https://github.com/verbb/navigation/issues/259#issuecomment-841743113

From my testing, this shouldn't be effecting overall performance.

There's a reason we don't use $schemaVersion = Navigation::getInstance()->schemaVersion; is because that's not looking at the current schemaVersion value stored in the project config, only the schemaVersion of the current code. We need this value when we're checking against upgrades (that Craft itself runs) which re-saves elements. This re-saving process will fire the queries to populate a navigation node. As such, in an upgrade scenario, we can't use this to detect if the schemaVersion has changed from the stored value before an upgrade to what it's about to go to (if updating Navigation).

VesninAndrey commented 3 years ago

Got it, good point.

I have come across custom code that has a lot of "Node" requests going on. Was thinking you could store the version in the cache, but faced cache invalidation problem (after plugin upgrade for example) =)

Thx for the clear answer.

engram-design commented 3 years ago

Still looking into some alternatives, but yeah, it's a tricker situation than it should be (if you follow the other thread)