harmen/hypha
1
0
Fork 0
Code Issues 142 Pull requests 4 Projects Releases Packages Wiki Activity

peer_reviewed_article: Add featured image #290

Closed
laurensmartina wants to merge 2 commits from article-excerpt-focus into master
pull from: article-excerpt-focus
merge into: harmen:master
Conversation 19 Commits 2 Files changed 6 +120 -1
laurensmartina commented 2020-02-24 12:28:43 +00:00 (Migrated from github.com)
Copy link

peer_reviewed_article: more focus on excerpt
peer_reviewed_article: added feature image

peer_reviewed_article: more focus on excerpt peer_reviewed_article: added feature image
matthijskooijman (Migrated from github.com) requested changes 2020-02-24 15:21:32 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

I left some comments inline.

Overall, I wonder about the duplication in code between the article edit form handling and the wymeditor upload handling. Can we somehow unify these two?

I can imagine that the editor does a background upload (like the wymeditor) and then puts the resulting url/filename in the edit form, rather than uploading the image as part of the edit form itself.

Alternatively, I can imagine that the code is slightly generalized and can be called from both the wymeditor upload handler as well as from the article edit form handler? Or maybe the HTMLForm class could support this somehow?

I left some comments inline. Overall, I wonder about the duplication in code between the article edit form handling and the wymeditor upload handling. Can we somehow unify these two? I can imagine that the editor does a background upload (like the wymeditor) and then puts the resulting url/filename in the edit form, rather than uploading the image as part of the edit form itself. Alternatively, I can imagine that the code is slightly generalized and can be called from both the wymeditor upload handler as well as from the article edit form handler? Or maybe the HTMLForm class could support this somehow?
system/assets/hypha-image-uploader.js
matthijskooijman (Migrated from github.com) commented 2020-02-24 14:53:03 +00:00
Copy link

This log should probably be removed.

This log should probably be removed.
matthijskooijman (Migrated from github.com) commented 2020-02-24 15:07:35 +00:00
Copy link

This probably needs some htmlencoding for the src attribute (probably easiest to generate an img with src and then use the jquery attribute set method to set the src).

This probably needs some htmlencoding for the src attribute (probably easiest to generate an img with src and then use the jquery attribute set method to set the src).
matthijskooijman (Migrated from github.com) commented 2020-02-24 15:11:06 +00:00
Copy link

What's the point of the accept attribute here? I could not find any info on it?

Also, this link and/or img should probably get a CSS class and the width/height should be in hypha.css.

What's the point of the `accept` attribute here? I could not find any info on it? Also, this link and/or img should probably get a CSS class and the width/height should be in hypha.css.
matthijskooijman (Migrated from github.com) commented 2020-02-24 15:12:30 +00:00
Copy link

Why bother with this when you're about to remove the preview entirely?

Why bother with this when you're about to remove the preview entirely?
matthijskooijman (Migrated from github.com) commented 2020-02-24 15:13:54 +00:00
Copy link

Maybe this line should be moved way further down, since the src var is only used there (and now it might be confusing with the src parameter to createPreviewImage().

Maybe this line should be moved way further down, since the `src` var is only used there (and now it might be confusing with the `src` parameter to `createPreviewImage()`.
system/core/pages.php
matthijskooijman (Migrated from github.com) commented 2020-02-24 14:54:34 +00:00
Copy link

I think this allows deleting arbitrary files, which is a security problem (e.g. consider posting with src=data/hypha.xml, or maybe even something outside of the docroot).

I think this allows deleting arbitrary files, which is a security problem (e.g. consider posting with `src=data/hypha.xml`, or maybe even something outside of the docroot).
matthijskooijman (Migrated from github.com) commented 2020-02-24 15:14:29 +00:00
Copy link

Also, this only deletes the resized image, not the original (if it exists).

Also, this only deletes the resized image, not the original (if it exists).
system/datatypes/peer_reviewed_article.php
@ -477,0 +506,4 @@
if ($needResize) {
image_resize($destinations[$orgUrlIndex], $destinations['img'], $maxSize[0], $maxSize[1]);
}
}
matthijskooijman (Migrated from github.com) commented 2020-02-24 15:00:26 +00:00
Copy link

This silently ignores images that are too big or have an invalid extension.

This silently ignores images that are too big or have an invalid extension.
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-02-24 15:02:48 +00:00
Copy link

This should be featured-image rather than excerpt?

This should be featured-image rather than excerpt?
laurensmartina (Migrated from github.com) reviewed 2020-03-02 10:36:01 +00:00
system/assets/hypha-image-uploader.js
laurensmartina (Migrated from github.com) commented 2020-03-02 10:36:01 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 10:37:30 +00:00
system/core/pages.php
laurensmartina (Migrated from github.com) commented 2020-03-02 10:37:30 +00:00
Copy link

oopsie. good point!

oopsie. good point!
tammoterhark (Migrated from github.com) reviewed 2020-03-02 10:44:07 +00:00
system/datatypes/peer_reviewed_article.php
tammoterhark (Migrated from github.com) commented 2020-03-02 10:44:07 +00:00
Copy link

Please: no inline styling...

Please: no inline styling...
laurensmartina (Migrated from github.com) reviewed 2020-03-02 10:55:06 +00:00
system/core/pages.php
laurensmartina (Migrated from github.com) commented 2020-03-02 10:55:06 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 11:01:07 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-03-02 11:01:07 +00:00
Copy link

@matthijskooijman : yes

@matthijskooijman : yes
laurensmartina (Migrated from github.com) reviewed 2020-03-02 11:03:35 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-03-02 11:03:35 +00:00
Copy link

@tammoterhark : is all over the file.. I consider this to be out of the scope of this pull request.

@tammoterhark : is all over the file.. I consider this to be out of the scope of this pull request.
laurensmartina (Migrated from github.com) reviewed 2020-03-02 11:13:33 +00:00
system/assets/hypha-image-uploader.js
laurensmartina (Migrated from github.com) commented 2020-03-02 11:13:32 +00:00
Copy link

Seems logical, but actually this is needed to work properly.

Seems logical, but actually this is needed to work properly.
laurensmartina (Migrated from github.com) reviewed 2020-03-02 12:39:17 +00:00
system/assets/hypha-image-uploader.js
laurensmartina (Migrated from github.com) commented 2020-03-02 12:39:17 +00:00
Copy link

placed the accept at the input field, where it belongs.

placed the accept at the input field, where it belongs.
laurensmartina (Migrated from github.com) reviewed 2020-03-02 12:39:59 +00:00
system/assets/hypha-image-uploader.js
laurensmartina (Migrated from github.com) commented 2020-03-02 12:39:59 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 12:49:02 +00:00
system/assets/hypha-image-uploader.js
laurensmartina (Migrated from github.com) commented 2020-03-02 12:49:02 +00:00
Copy link

moved styling to css

moved styling to css
laurensmartina (Migrated from github.com) reviewed 2020-03-02 13:12:13 +00:00
system/assets/hypha-image-uploader.js
laurensmartina (Migrated from github.com) commented 2020-03-02 13:12:13 +00:00
Copy link

found a way to fix it

found a way to fix it
laurensmartina (Migrated from github.com) reviewed 2020-03-02 13:12:34 +00:00
system/assets/hypha-image-uploader.js
laurensmartina (Migrated from github.com) commented 2020-03-02 13:12:34 +00:00
Copy link

fixed

fixed
laurensmartina commented 2020-03-02 13:26:40 +00:00 (Migrated from github.com)
Copy link

Overall, I wonder about the duplication in code between the article edit form handling and the wymeditor upload handling. Can we somehow unify these two?

I can imagine that the editor does a background upload (like the wymeditor) and then puts the resulting url/filename in the edit form, rather than uploading the image as part of the edit form itself.

Alternatively, I can imagine that the code is slightly generalized and can be called from both the wymeditor upload handler as well as from the article edit form handler? Or maybe the HTMLForm class could support this somehow?

I agree with all of you comments. And would like to fix this later on.

> Overall, I wonder about the duplication in code between the article edit form handling and the wymeditor upload handling. Can we somehow unify these two? > > I can imagine that the editor does a background upload (like the wymeditor) and then puts the resulting url/filename in the edit form, rather than uploading the image as part of the edit form itself. > > Alternatively, I can imagine that the code is slightly generalized and can be called from both the wymeditor upload handler as well as from the article edit form handler? Or maybe the HTMLForm class could support this somehow? I agree with all of you comments. And would like to fix this later on.
tammoterhark commented 2020-03-02 14:53:42 +00:00 (Migrated from github.com)
Copy link

Image upload to featured image seems to work at first, but after saving the article and re-opening, the image has disappeared.

When I look on disk, the image is there, but it just does not render. Showed to to Laurens.

Image upload to featured image seems to work at first, but after saving the article and re-opening, the image has disappeared. When I look on disk, the image is there, but it just does not render. Showed to to Laurens.
laurensmartina (Migrated from github.com) reviewed 2020-03-09 13:38:28 +00:00
system/datatypes/peer_reviewed_article.php
@ -477,0 +506,4 @@
if ($needResize) {
image_resize($destinations[$orgUrlIndex], $destinations['img'], $maxSize[0], $maxSize[1]);
}
}
laurensmartina (Migrated from github.com) commented 2020-03-09 13:38:27 +00:00
Copy link

fixed

fixed
matthijskooijman (Migrated from github.com) reviewed 2020-03-09 14:35:08 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

Some more comments, WIP.

Some more comments, WIP.
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-03-09 14:22:50 +00:00
Copy link

If $featuredImageData['error'] is set, then the upload is completely ignored, without a message to the user. Is that ok? When can this happen? Doesn't this include "File too big"?

If `$featuredImageData['error']` is set, then the upload is completely ignored, without a message to the user. Is that ok? When can this happen? Doesn't this include "File too big"?
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-03-09 14:24:16 +00:00
Copy link

Maybe use pathinfo here instead?

Maybe use `pathinfo` here instead?
system/datatypes/peer_reviewed_article.php
@ -466,14 +470,49 @@ class peer_reviewed_article extends HyphaDatatypePage {
return ['redirect', $this->constructFullPath($this->pagename)];
}
matthijskooijman (Migrated from github.com) commented 2020-03-09 14:20:45 +00:00
Copy link

Why append the upload max filesize, why not use the interpolation support that __ now has?

Why append the upload max filesize, why not use the interpolation support that __ now has?
system/datatypes/peer_reviewed_article.php
@ -473,4 +491,4 @@
if (!empty($form->errors)) {
return $this->editViewRender($request, $form);
}
matthijskooijman (Migrated from github.com) commented 2020-03-09 14:28:54 +00:00
Copy link

This still ignores a failure of move_uploaded_file or image_resize (though that might trigger a PHP error, of course, which might be sufficien).

This still ignores a failure of `move_uploaded_file` or `image_resize` (though that might trigger a PHP error, of course, which might be sufficien).
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-03-09 14:34:35 +00:00
Copy link

Why wait until here to unlink? If you unlink in the if above, you probably do not need the $orgImg variable at all (since before setting a new image you can look at the original value in the XML).

Also, this still unlinks only the resized version, not the original (if present).

Why wait until here to unlink? If you unlink in the if above, you probably do not need the `$orgImg` variable at all (since before setting a new image you can look at the original value in the XML). Also, this still unlinks only the resized version, not the original (if present).
laurensmartina (Migrated from github.com) reviewed 2020-03-09 14:42:14 +00:00
system/datatypes/peer_reviewed_article.php
@ -466,14 +470,49 @@ class peer_reviewed_article extends HyphaDatatypePage {
return ['redirect', $this->constructFullPath($this->pagename)];
}
laurensmartina (Migrated from github.com) commented 2020-03-09 14:42:14 +00:00
Copy link

is also like this in loadPage() in pages.php (case HyphaRequest::HYPHA_SYSTEM_PAGE_UPLOAD). this needs to be centralised and improved.

is also like this in `loadPage()` in `pages.php` (`case HyphaRequest::HYPHA_SYSTEM_PAGE_UPLOAD`). this needs to be centralised and improved.
matthijskooijman commented 2020-04-20 16:29:29 +00:00 (Migrated from github.com)
Copy link

I've copied the excerpt commit to #299, leaving the featured image commits here for more discussion and improvement. I also rebased this branch on top of #299, which is rebased on top of current master.

I've copied the excerpt commit to #299, leaving the featured image commits here for more discussion and improvement. I also rebased this branch on top of #299, which is rebased on top of current master.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
matthijskooijman
Labels
Clear labels   
Component: User interface

   
Component: Wymeditor

   
Help wanted

   
Level: Difficult

Structural changes, need discussion

  
Level: Easy

Under 4 hours of work

  
Level: Moderate

Under 16 hours of work

  
Pagetype: Festival

   
Pagetype: Mailinglist

   
Pagetype: Peer reviewed article

   
Pagetype: Text

   
Privacy GDPR AVG

   
status: has conflicts

   
Status: Needs changes

   
Status: Needs discussion

   
Status: Needs review

   
Status: Ready to merge

   
Status: Waiting for response

   
Type: Bug

   
Type: Enhancement

   
Type: Question

   
Usecase: De Stadsbron

   
Usecase: Koppelting

   
Usecase: MeetjeStad

   
Value: Coders

Enhancement of codebase, API, documentation

  
Value: Security

   
Value: Users

Improve user experience, add new features

  
Value: Visitors

Improve navigation, styling

No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
harmen/hypha!290
No description provided.