Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert behavior of image_width and image_height for media source images #13672

Merged
merged 1 commit into from Nov 10, 2017

Conversation

labr1005
Copy link
Contributor

@labr1005 labr1005 commented Nov 4, 2017

What does it do?

Prior to 2.6, the returned values of "image_width" and "image_height" were the size of the original image. 2.6 returns the size of the with phpthumb processed (and therefor often downscaled) image.

This reverts the behavior of "image_width" and "image_height" to the original state.

Why is it needed?

This PR does not affect MODX directly but fixes problems with extras that rely on and process these values - like ImagePlus.

Related issue(s)/PR(s)

Introduced in #13629
Reported in #13670

@Jako Jako added area-core bug The issue in the code or project, which should be addressed. priority-2-high labels Nov 4, 2017
@@ -1134,11 +1134,9 @@ public function getObjectsInContainer($path) {
'id' => $bases['urlAbsoluteWithPath'].$fileName,
'name' => $fileName,
'cls' => 'icon-'.$fileExtension,
'file_width' => $size[0],
'file_height' => $size[1],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why you're getting rid of these array items?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced them with my PR #13629. So they occurred first in 2.6.

My idea was to have separate variables for a) the width and height of the preview image in the MODX media browser (image_width and image_height) and b) for the display of the original image's size (file_width and file_height) displayed below the preview image.

But in my aforementioned PR image_width and image_height is the size of the with phpthumb processed (and therefor often downscaled) image.

My error was that I didn't take in account that other extras like ImagePlus rely on and process image_width and image_height.

So I basically only reverted image_width and image_height to the original state and therefor file_width and file_height are obsolet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or in short: I changed a variables inital purpose and this reverts it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mark-H Did this answer your question?

If no, anything else that I can clear up?

If yes, may I suggest considering this for 2.6.1 or the next patch? It's confirmed here #13670 and since I'm also responsible for the PR #13629 I can secure that there will be no problems with the new Media Browser features introduced with 2.6.

@Jako Jako added this to the v2.6.1 milestone Nov 10, 2017
@Jako Jako merged commit 1d4a2c9 into modxcms:2.6.x Nov 10, 2017
@labr1005 labr1005 deleted the fix-13670 branch November 8, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core bug The issue in the code or project, which should be addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants