Backup/restore of icons #9
Conversation
Backup/restore of icons
There was a problem hiding this comment.
@rogiervandongen Please see my comments and make changes as appropriate. I do appreciate that there is pure SQL already there and in the Moodle code but my understanding is that it should be avoided if at all possible. In addition there are other separate issues outside of the context of the pull that appear to be causing Code Checker to complain.
|
|
||
| public function __construct($plugintype, $pluginname, $optigroup, $step) { | ||
| parent::__construct($plugintype, $pluginname, $optigroup, $step); | ||
| } |
| // | ||
| // You should have received a copy of the GNU General Public License | ||
| // along with Moodle. If not, see <http://www.gnu.org/licenses/>. | ||
| /** |
There was a problem hiding this comment.
| // Customisation can be done for ALL activity types. | ||
| $courseiconwrapper = new backup_nested_element('courseicons'); | ||
| $courseicon = new backup_nested_element('courseicon', [], ['name']); | ||
| $sql = 'SELECT DISTINCT m.name FROM {modules} m'; |
There was a problem hiding this comment.
Do all supported databases support 'DISTINCT'? Would it not be better to use a DB API call instead of pure SQL as recommended (I believe I've seen this in the Moodle developer documentation)?
| $pluginwrapper->add_child($courseiconwrapper); | ||
| $courseiconwrapper->add_child($courseicon); | ||
|
|
||
| // This seems to work ere, we actually have the old course context available. |
There was a problem hiding this comment.
Seems to work? You don't sound sure.
| 'SELECT max(section) FROM {course_sections} WHERE course = ?', | ||
| [$this->step->get_task()->get_courseid()]); | ||
| $this->originalnumsections = (int)$maxsection; | ||
| 'SELECT max(section) FROM {course_sections} WHERE course = ?', |
There was a problem hiding this comment.
Ditto with previous SQL comment.
|
|
||
| // Create one standard named plugin element (the visible container) using the base recommended name. | ||
| $pluginwrapper = new backup_nested_element($this->get_recommended_name()); | ||
| // Connect to container |
| // GAWD, I hate backup/restore. Unclear, messy, lacking. Just... plain... wrong. | ||
| // ELOY, YOUR IMPLEMENTATION SUCKS! | ||
| $customicon = new backup_nested_element('modicon', [], ['itemid', 'modname', 'oldctxid']); | ||
| $customicon->set_source_array([['itemid' => 0, 'modname' => $this->task->get_modulename(), 'oldctxid' => $modulecontextid]]); |
| if ($sectionnum > $numsections && $sectionnum > $this->originalnumsections) { | ||
| $DB->execute("UPDATE {course_sections} SET visible = 0 WHERE course = ? AND section = ?", | ||
| [$this->step->get_task()->get_courseid(), $sectionnum]); | ||
| [$this->step->get_task()->get_courseid(), $sectionnum]); |
There was a problem hiding this comment.
Why the extra indentation?
| * | ||
| * @var array | ||
| */ | ||
| protected static $modicons = []; |
There was a problem hiding this comment.
Should this be defined at the top of the class? For maintainability and readability.
version.php
Outdated
| @@ -25,9 +25,9 @@ | |||
|
|
|||
| defined('MOODLE_INTERNAL') || die(); | |||
|
|
|||
| $plugin->version = 2024050801; | |||
| $plugin->version = 2024050802; | |||
There was a problem hiding this comment.
Why a version bump? There is nothing that actually needs it here. Also version bumping in a code change commit makes it more difficult to back port. Better to version bump a new development sequence, commit, then add code required in separate commits. Then even if code needs the version to be bumped then when the client installs the tagged release, this happens. Thus leaving only devs to cope, which they can by down then upgrading.
|
@gjb2048 If a database wouldn't even be able to use the most basic SQL definitions, they cannot realistically be considered a usable database. This goes for DISTINCT (which is heavily used in Moodle's core), MIN/MAX functions, etc etc. So yeah, they're supported. By every database Moodle supports (which aren't that many). Version bump removed. Last but not least, added "global $DB" in vsf_after_restore_course_numsections() as it was missing. |
|
@rogiervandongen Thanks, but where is the updated commit please? Fair enough with the SQL reply, I just did have to ask the question. And totally with the coding standards, drives me nuts some days! |
|
@gjb2048 Frankly, I've never seen that many non-sensical coding standards which the company that defines them is the exact same company that circumvents nearly every one of them 😂 "The joys of Moodle" (one of the worst to me is forbidding protected variables that are "underscore prefixed". The so called persistence model could be so much easier. From the moment I've started coding in PHP, which was somewhere in '07, all my business objects and models depended on "protected $_a", with getters and setters that then rely on magic __get, __set, __isset and __unset implementations that make for a much better to understand implementation. It also makes PHP's new Attributes implementation more than just easy to facilitate, aside from the fact actually implementing any logic using those attributes is terribly slow, unfortunately). |
gjb2048
merged commit 2c9c69c
into
Lesterhuis-Training-en-Consultancy:master
|
@rogiervandongen Thanks, merged, the actual 'release' in version.php does need to go back, but I can do that as it seems I also have some code checker things to do on my code! Grrr! And, yeah with coding standards, nuff said! |
Backup/restore of icons