[#7060] - Fixes to Add fast check for image orientation (#3568)
- Closed
- 3 Jun 2015
- Medium
- Build: staging
- # 7060
- Diff
- smz:NotAnotherHelperClass
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Description
This fixes some issues from #3568:
- it get rids of the new (yet-to-be-released!) JImageHelper class by incorporating its code into the JImage class.
- it correct some comment blocks
Test instruction
Assuming you are using the Protostar template, modify the template's index.php by inserting the following code after line 27 ($sitename = $app->get('sitename');):
echo '<pre>';
// Define the file to use.
// NOTE: Make sure this file exists or use another path to an existing image file!
$file = JPATH_BASE . '/images/powered_by.png';
echo str_repeat('_', 60) . "\n\n";
echo 'File is: ' . print_r($file, true) . "\n";
echo str_repeat('_', 60) . "\n\n";
// Get instance of JImage.
$image = new JImage($file); // $image must be instance of JImage
$orientation = $image->getOrientation();
echo 'For the $image object orientation is: ' . print_r($orientation, true) . "\n";
echo(str_repeat('_', 60)) . "\n\n";
// Get image properties calling static method from JImage::getImageFileProperties().
$props = JImage::getImageFileProperties($file);
echo 'Properties from JImage::getImageFileProperties($file);:';
echo "\n\n";
print_r($props);
echo "\n";
echo (str_repeat('_', 60)) . "\n\n";
echo 'Check for every property separately:' . "\n\n";
// Check the returned data contains all of these properties.
foreach (array(
'width',
'height',
'type',
'attributes',
'bits',
'channels',
'mime',
'filesize',
'orientation') as $p
)
{
if (array_key_exists($p, $props))
{
echo "{$p} is: " . print_r($props->$p, true) . "\n";
}
else
{
echo print_r("\$props does not contain {$p}-data", true) . "\n";
}
}
echo(str_repeat('_', 60)) . "\n";
echo '</pre>';
die;
Visit your test site. The expected output is:
____________________________________________________________
File is: _PATH_TO_YOUR_JPATH_BASE_HERE_/images/powered_by.png
____________________________________________________________
For the $image object orientation is: landscape
____________________________________________________________
Properties from JImage::getImageFileProperties($file);:
stdClass Object
(
[width] => 150
[height] => 45
[type] => 3
[attributes] => width="150" height="45"
[bits] => 8
[channels] =>
[mime] => image/png
[filesize] => 4048
[orientation] => landscape
)
____________________________________________________________
Check for every property separately:
width is: 150
height is: 45
type is: 3
attributes is: width="150" height="45"
bits is: 8
channels is:
mime is: image/png
filesize is: 4048
orientation is: landscape
____________________________________________________________
Should this be tested or is it OK to just code review it?
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Labels |
Added:
?
|
||
I don't think there is a need for a new method name. Just put the code of the method into the existing getOrientation. Instead of checking the two passed in arguments, you check the class properties.
That's the difference between a helper where you pass the dimension in to a static method as arguments or you use a OOP class where the dimensions are properties of the object.
The new private getOrientationString() function is used in getImageFileProperties() too, so I think there is a good point for it being a function...
... and thanks for clarifying the role of helper classes: very clear now!!
The new private getOrientationString() function is used in getImageFileProperties() too, so I think there is a good point for it being a function...
True, didn't catch that one.
and thanks for clarifying the role of helper classes: very clear now!!
At least it's how I understand it. 
| Category | ⇒ | Libraries |
| Status | New | ⇒ | Pending |
@smz can you just put some testinstructions here? I guess we have only smal different than here: #3568?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7060.
)
Important:
I suggest to assign this PR a 3.4.2 milestone, even if not yet RTC: if we don't merge this before 3.4.2 release we are going to introduce a new class with which we'll must live till 4.0...
I decided to test this and apparently there are several issues: please stand-by before testing... 
... and for what is worth, @test OK by me, but... is it correct that for /images/powered_by.png we don't have "channels"?
Edit: probably yes, as it is a PNG with transparency.
For a normal JPEG image 3 is returned, which is correct.
____________________________________________________________
File is: (mypathere)/images/sampledata/fruitshop/bananas_2.jpg
____________________________________________________________
For the $image object orientation is: portrait
____________________________________________________________
Properties from JImage::getImageFileProperties($file);:
stdClass Object
(
[width] => 300
[height] => 352
[type] => 2
[attributes] => width="300" height="352"
[bits] => 8
[channels] => 3
[mime] => image/jpeg
[filesize] => 17313
[orientation] => portrait
)
____________________________________________________________
Check for every property separately:
width is: 300
height is: 352
type is: 2
attributes is: width="300" height="352"
bits is: 8
channels is: 3
mime is: image/jpeg
filesize is: 17313
orientation is: portrait
____________________________________________________________
| Milestone |
Added: |
||
@test
PR test sucessful
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7060.
| Status | Pending | ⇒ | Ready to Commit |
RTC based on testing. Thanks
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7060.
| Labels |
Added:
?
|
||
Merged into staging. Thanks.
| Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-06-03 10:57:38 |
| Closed_By | ⇒ | Bakual |
| Labels |
Removed:
?
|
||
@Bakual, @itbra
I had to introduce a new
getOrientationString()function as we had twogetOrientation()functions in two different classes doing two different thingssince #3568 extended the amount of information returned by
getImageFileProperties()and added the new publicgetOrientation()function, shouldn't we modify test units accordingly?