issue #27 #28

ewallah · 2021-05-10T14:02:47Z

Fix Behat integration.

ewallah · 2021-05-10T14:20:51Z

The result of this change can be seen in github workflow

gthomas2

Thank you very much for your pull request.
I've left some comments as to some things that need to be changed.

gthomas2 · 2021-05-11T07:23:58Z

tests/filter_test.php

@@ -87,7 +87,7 @@ public function test_empty_image() {
        ];
        foreach ($sizes as $size) {
            $emptyimage = phpunit_util::call_internal_method($filter, 'empty_image', $size, get_class($filter));
-            $this->assertContains('width="'.$size[0].'" height="'.$size[1].'"', $emptyimage);
+            $this->assertStringContainsString('width="'.$size[0].'" height="'.$size[1].'"', $emptyimage);


Hey, thanks for the pull request. Are there any benefits to changing this bit of code? I've taken a look at the original assertContains method and it works fine with strings. The only benefit I can see to using assertStringContainsString over assertContains is that it is more explicit in the type its testing for.

PHP 7.4 retired December 6 2021, PHP 8.0 is much stricter.

Otherwise you get a TypeError: PHPUnit\Framework\Assert::assertContains(): Argument $haystack must be of type iterable, string given.

Thanks for the explanation, that makes total sense.

gthomas2 · 2022-01-04T18:59:35Z

classes/componentsupport/mod_page.php

    private static function check_component(array $pathcomponents) {
        if ($pathcomponents[1] !== 'mod_page') {
            throw new \coding_exception('Component is not a page ('.$pathcomponents[2].')');
        }
    }

+    /**


This comment should not be used since the abstract base_component documents this function.
Moodle coding standards state that functions should not be documented if a base class already has a doc block.
You can add the following comment to clarify this if you wish:

// Inherited.

gthomas2 · 2022-01-04T19:00:03Z

classes/componentsupport/mod_page.php

+     * @param array $pathcomponents
+     * @param int $maxwidth
+     * @return string
+     */


Inherited method should not have comment.

33 Errors https://moodle.org/plugins/filter_imageopt/3.9.0.0/24112?smurf=html#phpdoc
vs
0 Errors https://github.com/ewallah/moodle-filter_imageopt/runs/4702891371?check_suite_focus=true#step:9:1

I'm just going by Moodle's documented coding standards which state:

"You must include a description even if it appears to be obvious from the @param and/or @return lines.

An exception is made for overridden methods which make no change to the meaning of the parent method and maintain the same arguments/return values. In this case you should omit the comment completely"

https://docs.moodle.org/dev/Coding_style#Functions

I wonder if those errors would show if we documented the functions in an interface instead of the base class?

BTW - I do really appreciate your work on this.

gthomas2 · 2022-01-04T19:00:08Z

classes/componentsupport/mod_page.php

@@ -73,6 +99,13 @@ public static function get_optimised_path(array $pathcomponents, $maxwidth) {
        return $optimisedpath;
    }

+    /**


Inherited method should not have comment.

gthomas2 · 2022-01-04T19:00:37Z

classes/componentsupport/question.php

 class question extends base_component {

+    /**


Inherited method should not have comment.

gthomas2 · 2022-01-04T19:02:18Z

filter.php

@@ -244,7 +258,7 @@ private function apply_loadonvisible(array $match, stored_file $file, $originals
        if (!$imageinfo || !isset($imageinfo->width)) {
            return ($img);
        }
-        $width = $imageinfo->width;
+        $width = $imageinfo->width > $maxwidth ? $maxwidth : $imageinfo->width;
        $height = $imageinfo->height;


If we go over the maximum width then we should adjust the height in ratio to the adjusted width.

Why calculating max width in this function when you do not use it?

But yes, the height should be also adjusted.

It will take me a while to familiarise myself with the code before I can answer that - I wrote it about 5 years ago!

OK - I've had a look at the code base and this is what I've found:

apply_loadonvisible only replaces the "<img" portion which should leave any width / height attributes intact.

The maxwidth variable as you stated is not used - it should actually be removed. That's the fix we should go with.

The maxwidth is actually handled in img_add_width_height so we don't need to worry about it in apply_loadonvisible.

gthomas2 · 2022-01-04T19:02:44Z

tests/behat/behat_filter_imageopt.php

 * @author    Guy Thomas <[email protected]>
 * @copyright Copyright (c) 2017 Guy Thomas.
 * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
 */
 class behat_filter_imageopt extends behat_base {
    /**
+     * If image optimiser is eneabled.


typo on "eneabled"

gthomas2 · 2022-01-04T19:03:27Z

tests/behat/behat_filter_imageopt.php

 * @author    Guy Thomas <[email protected]>
 * @copyright Copyright (c) 2017 Guy Thomas.
 * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
 */
 class behat_filter_imageopt extends behat_base {
    /**
+     * If image optimiser is eneabled.


Note - you don't need to add additional doc block comments when the @given / @when / @then is clear enough.

gthomas2 · 2022-01-04T19:30:23Z

tests/filter_test.php

@@ -104,26 +107,26 @@ public function test_image_opt_url() {
        set_config('maxwidth', '480', 'filter_imageopt');

        $fixturefile = 'testpng_2880x1800.png';
-        $context = context_system::instance();
+        $context = \context_system::instance();


Tests aren't namespaced and therefore you don't need to add a backslash before the classes.

This goes for every other inserted backslash before a class in this file.

AH - I am wrong. I see that you have inserted a namespace at the top of the file. Please try to add use statements to avoid repeated backslashing.

E.g. after "defined('MOODLE_INTERNAL') || die();"

use context_system

gthomas2 · 2022-01-04T19:31:23Z

tests/privacy/privacy_test.php

+/**
+ * Tests for filter class
+ * @package   filter_imageopt
+ * @author    Guy Thomas <[email protected]>


If you're adding a file please add yourself as the author since you deserve credit for your work.

ewallah added 3 commits May 10, 2021 16:01

issue gthomas2#27

b0599d3

prepare for phpunit9

df43256

workflow

5095cd8

rdebleu added 2 commits January 4, 2022 13:19

code review

000d85a

code review

94092ad

gthomas2 requested changes Jan 4, 2022

View reviewed changes

rdebleu added 6 commits March 12, 2023 23:44

grunt

ac99ad6

cbf checks

10fdcb8

code review

7c24a59

wrong namespace

f5e7f3b

php 8.1

ec870b7

workflow

92c461c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #27 #28

issue #27 #28

ewallah commented May 10, 2021

ewallah commented May 10, 2021

gthomas2 left a comment

gthomas2 May 11, 2021

ewallah Jan 5, 2022

gthomas2 Jan 5, 2022

gthomas2 Jan 4, 2022

gthomas2 Jan 4, 2022

ewallah Jan 11, 2022

gthomas2 Jan 12, 2022

gthomas2 Jan 4, 2022

gthomas2 Jan 4, 2022

gthomas2 Jan 4, 2022

ewallah Jan 6, 2022

gthomas2 Jan 6, 2022

gthomas2 Jan 12, 2022

gthomas2 Jan 4, 2022

gthomas2 Jan 4, 2022

gthomas2 Jan 4, 2022

gthomas2 Jan 4, 2022

gthomas2 Jan 5, 2022

gthomas2 Jan 4, 2022

issue #27 #28

Are you sure you want to change the base?

issue #27 #28

Conversation

ewallah commented May 10, 2021

ewallah commented May 10, 2021

gthomas2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment