Fix roles file comparison to use object not string #878
Conversation
classes/OpenXdmod/Migration/Version800To810/ConfigFilesMigration.php
Outdated
Show resolved
Hide resolved
plessbd
force-pushed
the
fix-cloud-charts
branch
3 times, most recently
from
e622509 to
cf3861f
Compare
plessbd
force-pushed
the
fix-cloud-charts
branch
from
cf3861f to
6226300
Compare
There was a problem hiding this comment.
This does raise an interesting design question: how much effort should we expend in migrating old config files that have been edited by the user. In this pull request we add the summary charts to the default role - but what should we do if the user has already added some alternative summary charts. Should we merge the charts? Merge and deduplicate them? Overwrite the user's charts? Leave the edited charts alone?
My overall preference is that we move to a new paradigm where if a user has edited a configuration file then we do not overwrite the edits and provide sufficient documentation to allow the user to do the merge. Afterall, they are the only ones who know what the original intent and reason for the original edits to the old files - we are not going to be able to guess their intentions. Of course, If the original file has not been edited then it can be safely replaced with the new version
|
I agree with @jpwhite4 and my preference is to follow a similar paradigm as system installs where if the file has been changed, we place a config.json.new file in the directory (it won't be picked up by the Configuration scanner) and let the user sort it out. Migrating configuration files should not be an area where we expend any significant effort. |
|
I agree. Do we currently have a way of knowing if they edited the file? Since all we have is the "current" file. I would say some of these files should have an "enabled" key so that the file doesn't have to have objects deleted, just enabled or disabled, then we would be better able to do some of this. |
| return file_get_contents(TEMPLATE_DIR . '/roles.d/' . $realm . '.json') | ||
| === file_get_contents($rolesFile); | ||
| return JSON::loadFile(TEMPLATE_DIR . '/roles.d/' . $realm . '.json', false) | ||
| === JSON::loadFile($rolesFile, false); |
There was a problem hiding this comment.
Two different object instances will never be === (https://www.php.net/manual/en/language.oop5.object-comparison.php).
smgallo
changed the title
Made it so roles.d/cloud.json is updated properly
Fixed comparison to be based on the object not string