Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the behaviour of $forceLog parameter for Mage::log() #3263

Merged
merged 1 commit into from
May 18, 2023
Merged

Fixed the behaviour of $forceLog parameter for Mage::log() #3263

merged 1 commit into from
May 18, 2023

Conversation

empiricompany
Copy link
Contributor

Some extensions use Mage::log to enable custom debug log files, but even if they set $forceLog = true, the log won't be written if the level is lower (e.g. Zend_Log::INFO) and Magento is not in developer mode.

@github-actions github-actions bot added the Mage.php Relates to app/Mage.php label May 15, 2023
@fballiano
Copy link
Contributor

interesting! it happened many times that I had logs disabled, wanted to log something and it didn't work... I've to think if there could be some problems with this :-)

@empiricompany
Copy link
Contributor Author

empiricompany commented May 15, 2023

Perhaps it would be better to limit the error level and developer mode checks only to default Magento log file (system.log) and not to custom log files.
Many extensions use custom debug logs without the $forceLog = true parameter, and they wouldn't be written anyway without developer mode enabled.

This code seems a bit confusing to me.

        try {
            $logActive = self::getStoreConfig('dev/log/active');
            if (empty($file)) {
                $file = self::getStoreConfig('dev/log/file');
            }
        } catch (Exception $e) {
            $logActive = true;
        }

        if (!self::$_isDeveloperMode && !$logActive && !$forceLog) {
            return;
        }

        static $loggers = [];

        try {
            $maxLogLevel = (int) self::getStoreConfig('dev/log/max_level');
        } catch (Throwable $e) {
            $maxLogLevel = Zend_Log::DEBUG;
        }

        $level  = is_null($level) ? Zend_Log::DEBUG : $level;

        if (!self::$_isDeveloperMode && $level > $maxLogLevel && !$forceLog) {
            return;
        }

        $file = empty($file) ?
            (string) self::getConfig()->getNode('dev/log/file', Mage_Core_Model_Store::DEFAULT_CODE) : basename($file);

@fballiano
Copy link
Contributor

I think not having the forcelog is the right thing, if logs are disabled nothing should be logged, unless it's forcelog (maybe it's something extremely important that should be logged anyway while we don't have to have a million php notices logged) so I think the sentiment of te PR is correct :-)

@empiricompany
Copy link
Contributor Author

I think not having the forcelog is the right thing, if logs are disabled nothing should be logged, unless it's forcelog (maybe it's something extremely important that should be logged anyway while we don't have to have a million php notices logged) so I think the sentiment of te PR is correct :-)

Yes, but for me it would be better if for custom logs other than system.log only check if logging is disabled. For custom logs, I would prefer not to check the developer mode or level.
Because logs can also be used to keep track of other things besides development in production.

@fballiano
Copy link
Contributor

mmmmm on that case I don't necessarily agree. I know a lot of extensions that log whatever kind of stuff that I wouldn't want to log if logs are disabled.

I see your point tho, but maybe (I'll still think about it) I prefer that logs are written only if forcelog is used :-)

@empiricompany
Copy link
Contributor Author

mmmmm on that case I don't necessarily agree. I know a lot of extensions that log whatever kind of stuff that I wouldn't want to log if logs are disabled.

I see your point tho, but maybe (I'll still think about it) I prefer that logs are written only if forcelog is used :-)

yes only check if log is not disabled or is a forced log, but remove developer mode and level for custom logs

fballiano

This comment was marked as duplicate.

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested, works perfectly

Copy link
Member

@elidrissidev elidrissidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh so this why sometimes the forceLog won't do anything!

@fballiano
Copy link
Contributor

probably 10+ years old bug heheheh

@fballiano fballiano changed the title forcing log, ignoring developer mode and error level. Fixed the behaviour of $forceLog parameter for Mage::log() May 18, 2023
@fballiano fballiano merged commit 6b08b60 into OpenMage:main May 18, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mage.php Relates to app/Mage.php
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants