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 return value of saveCache method #583

Merged
merged 1 commit into from
Apr 27, 2020
Merged

fixed return value of saveCache method #583

merged 1 commit into from
Apr 27, 2020

Conversation

sprankhub
Copy link
Contributor

The original method did not return a boolean value, but the return value of Mage_Core_Model_App::saveCache. See https://github.com/OpenMage/magento-mirror/blob/fe6a94e281cb3e8e166f4eece31c4df1faa8cec6/app/code/core/Mage/Core/Model/Layout/Update.php#L198.

@kkrieger85 kkrieger85 added the review needed Problem should be verified label Jun 2, 2019
@tmotyl tmotyl requested a review from colinmollenhour April 24, 2020 19:52
@tmotyl
Copy link
Contributor

tmotyl commented Apr 24, 2020

@colinmollenhour the other change was yours, so please review it. For me it looks ok.

@colinmollenhour
Copy link
Member

This method is only ever called once and the return value is ignored in that case. Also the MCMApp#saveCache method just returns $this so it will always be the same value and has no special significance other than it is not "false" which is returned when the cache is disabled. So I don't really see a reason to change it..

@sprankhub
Copy link
Contributor Author

There may be others depending on that return value. Changing this just does not make any sense IMHO. It is just not proper OOP to completely change a return value (even the type!).

@colinmollenhour colinmollenhour merged commit 7948e61 into OpenMage:1.9.3.x Apr 27, 2020
@sprankhub sprankhub deleted the patch-1 branch April 28, 2020 06:42
@sreichel sreichel added bug and removed review needed Problem should be verified labels May 9, 2020
Sekiphp pushed a commit to Sekiphp/magento-lts that referenced this pull request Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants