[#14402] - [CS] Fixing JImage constants
- Closed
- 23 Jul 2018
- Medium
- Build: staging
- # 14402
- Diff
- piotr-cz:cs/jimage-constants
User tests: Successful: Unsuccessful:
Summary of Changes
- Switched from hardcoded variables to class constants when checking for crop type
- Using
staticinstead ofselfin methods
This is a coding style change and doesn't have any effects on functionality.
Only change would if one would extend the JImage class: developer doesn't have to define JImage constants again in extended class.
Testing Instructions
The only place where JImage is used in J! core is template editing feature:
- Go to Extensions > Templates > Templates > Protostar Details and Files
- Click on template_preview.png
- Make sure cropping and resizing works as expected
Expected result
Unit suites should pass
Manual tests pass
Actual result
same as expected result
Documentation Changes Required
No changes required
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
What about scenario when I'd extend the JImage class and let's say add extra information in the getImageFileProperties method?
Any code that would call self::getImageFileProperties method would actually call the JImage class, not the extended one at least as I understand it
The method calls are fine, it's the constants that I don't think this works on.
Looks like you are right.
Constants shouldn't be overwritten in subclass and so referring to them with static keyword is misleading.
| Labels |
Added:
?
|
||
I've kept static keyword only when referencing methods, however there is one more case - reference to $formats array
That one's good, it's not private. Looks fine to me.
Issue can be closed since the modified class is deprecated.
See /libraries/vendor/joomla/image/src:19
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14402.
Thanks, @jonasgonka
Superseeded by joomla-framework/image#19
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-07-23 11:31:28 |
| Closed_By | ⇒ | piotr-cz |
From the looks of things, there isn't actually anything gained by calling
static::CONSTANTbecause it's still referring either to itself or a parent class. It's not like late static bindings where it can resolve to a subclass if that's within the class chain, and https://secure.php.net/manual/en/language.oop5.constants.php#104260 seems to support that.