verbb / navigation

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

In verbb\navigation\services\navs::handleChangedNav() possible edge case results in error #266

Closed lthurston closed 3 years ago

lthurston commented 3 years ago

Description

We've encountered a case where $oldRecord->siteSettings is not set (see api/vendor/verbb/navigation/src/services/Navs.php line 317), resulting in a foreach loop over a null value on line 321:

       // When enabling/disabling sites
        if (Craft::$app->getIsMultiSite()) {
            // Has the sites been changed?
            $oldSiteSettings = Json::decode($oldRecord->siteSettings);
            $newSiteSettings = Json::decode($navRecord->siteSettings);

            // Removed sites
            foreach ($oldSiteSettings as $key => $value) {
                if (!isset($newSiteSettings[$key])) {
                    // Nothing for now
                }
            }

$oldSiteSettings should be checked to verify it's an array, or typecast to enforce that it's an array.

Steps to reproduce

This is difficult to provide clear information on, but don't think steps to reproduce are necessary to see that it could happen and therefore guardrails should be in place to prevent it.

Additional info

I'll provide a PR.

lthurston commented 3 years ago

Well, I'll be damned, this is fixed already. Not sure why we don't have this version of the plugin, but that's a me problem. Disregard!

engram-design commented 3 years ago

Yep, sorry about that, just waiting on some other support requests!

engram-design commented 3 years ago

Fixed in 1.4.19