First cleaning of the plugin #1
Conversation
There was a problem hiding this comment.
Salut Martin,
De très bon changements et optimisations en général. Le seul bémol: il faudrait tester cela avec le feed ici https://www.imt.fr/feed/ car il y a quelques erreurs fatales dues à des changements de types.
block_rss_thumbnails.php
Outdated
|
|
||
| /** @var bool track whether any of the output feeds have recorded failures */ | ||
| private $hasfailedfeeds = false; | ||
| private bool $hasfailedfeeds = false; |
There was a problem hiding this comment.
Attention ici on contraint les utilisateurs à utiliser PHP 7.4. C'est désiré mais pas forcément le cas pour tout le monde et jusqu'à Moodle 3.11, on ne requiers que du 7.3. Sachant qu'en PHP 7.3 la déclaration ici va empêcher le bloc de fonctionner.
There was a problem hiding this comment.
Oui, je me souviens que j'avais commencé à enlever les types, mais j'ai dû en oublier, je me mets dessus
| } | ||
|
|
||
| // initalise block content object | ||
| // Initialise block content object. |
block_rss_thumbnails.php
Outdated
| * @return block_rss_client\output\footer|null The renderable footer or null if none should be displayed. | ||
| */ | ||
| protected function get_footer($feedrecords) { | ||
| protected function get_footer($feedrecords) : block_rss_client\output\footer { |
There was a problem hiding this comment.
Là aussi voir la compatibilité PHP. A mon avis c'est bon car c'est du PHP 7.3+.
There was a problem hiding this comment.
Je vais me renseigner
block_rss_thumbnails.php
Outdated
| * @return block_rss_client\output\feed|null The renderable feed or null of there is an error | ||
| */ | ||
| public function get_feed($feedrecord, $maxentries, $showtitle) { | ||
| public function get_feed($feedrecord, $maxentries, $showtitle) : ?block_rss_client\output\feed { |
There was a problem hiding this comment.
La et au dessus: tu as importé le namespace pour avoir la classe feed, donc le mieux c'est d'utiliser "feed" aussi ici.
| */ | ||
| public function __construct($carousselspeed, array $feeds = array()) { | ||
| $this->feeds = $feeds; | ||
| parent::__construct($feeds); |
classes/output/item.php
Outdated
| protected $imageurl = null; | ||
| protected $categories = null; | ||
| protected ?string $imageurl = null; | ||
| protected ?string $categories = null; |
There was a problem hiding this comment.
Ici c'est un tableau ! Tu verras, une erreur fatale se produit ici.
There was a problem hiding this comment.
Bizarre que j'aie mis ça haha, surtout que je savais que c'est un tableau
block_rss_thumbnails.php
Outdated
| * @return block_rss_client\output\footer|null The renderable footer or null if none should be displayed. | ||
| */ | ||
| protected function get_footer($feedrecords) { | ||
| protected function get_footer($feedrecords) : block_rss_client\output\footer { |
There was a problem hiding this comment.
Attention: c'est ?block_rss_client\output\footer (sinon une erreur fatale se produit)
|
|
||
| defined('MOODLE_INTERNAL') || die(); | ||
|
|
||
| $plugin->version = 2021012200; // The current plugin version (Date: YYYYMMDDXX). |
There was a problem hiding this comment.
Tu peux incrémenter la version ici, surtout si tu as changé un language string (ça rafraîchira le cache).
|
|
||
| // Insert a new RSS Feed. | ||
| require_once("{$CFG->libdir}/simplepie/moodle_simplepie.php"); | ||
| require_once("$CFG->libdir/simplepie/moodle_simplepie.php"); |
There was a problem hiding this comment.
Là il y a un soucis dû à autre chose que ton changement. Il faudrait déclarer la variable $CFG.
* Test get feed method * Refactor plugin and documentation
martin-call-learning
force-pushed
the
master
branch
from
516998c to
ff0eaee
Compare
Still some issues to fix.
This pull request if you have some time so you can see what i've done and so our meeting tomorrow would be even more interesting. ;)
As I told you in Matrix there is this issue remaining :
Potentially polymorphic call. renderer_base does not have members in its hierarchyAnd another wich comes from an higher class
And some imports that are unknown but I think I have to lay it as it was