issue #27#28
Conversation
|
The result of this change can be seen in github workflow |
gthomas2
left a comment
gthomas2
left a comment
There was a problem hiding this comment.
Thank you very much for your pull request.
I've left some comments as to some things that need to be changed.
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for the explanation, that makes total sense.
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
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.
| * @param array $pathcomponents | ||
| * @param int $maxwidth | ||
| * @return string | ||
| */ |
There was a problem hiding this comment.
Inherited method should not have comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
| return $optimisedpath; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Inherited method should not have comment.
| */ | ||
| class question extends base_component { | ||
|
|
||
| /** |
There was a problem hiding this comment.
Inherited method should not have comment.
| } | ||
| $width = $imageinfo->width; | ||
| $width = $imageinfo->width > $maxwidth ? $maxwidth : $imageinfo->width; | ||
| $height = $imageinfo->height; |
There was a problem hiding this comment.
If we go over the maximum width then we should adjust the height in ratio to the adjusted width.
There was a problem hiding this comment.
Why calculating max width in this function when you do not use it?
But yes, the height should be also adjusted.
There was a problem hiding this comment.
It will take me a while to familiarise myself with the code before I can answer that - I wrote it about 5 years ago!
There was a problem hiding this comment.
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.
| */ | ||
| class behat_filter_imageopt extends behat_base { | ||
| /** | ||
| * If image optimiser is eneabled. |
| */ | ||
| class behat_filter_imageopt extends behat_base { | ||
| /** | ||
| * If image optimiser is eneabled. |
|
|
||
| $fixturefile = 'testpng_2880x1800.png'; | ||
| $context = context_system::instance(); | ||
| $context = \context_system::instance(); |
There was a problem hiding this comment.
Tests aren't namespaced and therefore you don't need to add a backslash before the classes.
There was a problem hiding this comment.
This goes for every other inserted backslash before a class in this file.
There was a problem hiding this comment.
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
| /** | ||
| * Tests for filter class | ||
| * @package filter_imageopt | ||
| * @author Guy Thomas <brudinie@gmail.com> |
There was a problem hiding this comment.
If you're adding a file please add yourself as the author since you deserve credit for your work.
3 participants
Fix Behat integration.