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

Improve tpl_getMediaFile() #2684

Merged
merged 3 commits into from May 31, 2020

Conversation

geekitude
Copy link
Contributor

Previously, if no candidate is found, the result would still always be last candidate url even if it doesn't exist (and function would trigger a Warning for trying to getimagesize() on a file that doesn't exist)

Copy link
Collaborator

@phy25 phy25 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I believe several places that use tpl_getMediaFile() should be looked at as well, or they will simply inject false into HTML. Also docs on the top of the function needs to be updated as well; it will not @return string only now.

Some occurrences of tpl_getMediaFile():

https://github.com/splitbrain/dokuwiki/blob/cd7e1573e9ecf4c6d8d11dafcad2483b342be55a/feed.php#L62

https://github.com/splitbrain/dokuwiki/blob/cd7e1573e9ecf4c6d8d11dafcad2483b342be55a/inc/template.php#L1757-L1780

https://github.com/splitbrain/dokuwiki/blob/1121b972bffbf7afb0aef5dfb54f8857c73c3c56/lib/tpl/dokuwiki/tpl_header.php#L23-L30

@Klap-in
Copy link
Collaborator

Klap-in commented Mar 13, 2019

Current usages in Dokuwiki, downloadable plugins and templates of this function:
Code search: tpl_getMediaFile

@splitbrain
Copy link
Collaborator

@geekitude will you update the PR with the changes requested by @phy25 ?

@geekitude
Copy link
Contributor Author

Sorry, I was a bit overwhelmed by real life.
I'm relly not sure on what would be a good way to solve the problem.

Do you think that providing a default transparent image would be a good solution? I believe it would avoid empty/non-existing result all at once?

@phy25
Copy link
Collaborator

phy25 commented Aug 18, 2019

Do you think that providing a default transparent image would be a good solution? I believe it would avoid empty/non-existing result all at once?

Sound good! I would add a new parameter $fallback with the default pointing to "lib/images/blank.gif" (may need an absolute path). Functions that still wants false can pass that parameter with false.

If you don't have the time to polish this I can go ahead and make the change (and still keep your credit) 😉

@geekitude
Copy link
Contributor Author

Thanks for your help. I'll give it a try soon and let you know if I need help 😉

@geekitude
Copy link
Contributor Author

Followed @phy25 idea to add a $fallback parameter that points to "lib/images/blank.gif" (or any other image provided as parameter) or that will stop process if false value is provided

I don't know what to do with The Travis CI build failure :(

Copy link
Collaborator

@phy25 phy25 left a comment

Choose a reason for hiding this comment

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

Thanks! For the build failure you may need to rebase your commits. master tests are fixed just recently.

If you need help I can rebase this for you...

inc/template.php Outdated Show resolved Hide resolved
@geekitude
Copy link
Contributor Author

Thanks! For the build failure you may need to rebase your commits. master tests are fixed just recently.

If you need help I can rebase this for you...

I'm sorry but I don't know what 'rebase your commit' means, so, your help for that would be much appreciated :)

Previously, if no candidate is found, the result would still always be last candidate url even if it doesn't exist (and function would trigger a Warning for trying to getimagesize() on a file that doesn't exist)
Here is a solution attempt, sorry for the long delay.
According to my tests, absolute path isn't required
@phy25
Copy link
Collaborator

phy25 commented Oct 21, 2019

@geekitude I rebased your branch locally, and force-pushed it to GitHub. Tests should be passing very soon :-)

Kind of applied given suggestion but switched $fallback to boolean to reduce possible values.
@geekitude
Copy link
Contributor Author

@phy25 thanks again for your help, I hope the latest version with boolean $fallback works better :)

Copy link
Collaborator

@phy25 phy25 left a comment

Choose a reason for hiding this comment

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

Looks good. How to implement $fallback (string or bool) is more of a design decision, but the current code is OK to merge.

Again, thanks for your contributions and patience!

@geekitude
Copy link
Contributor Author

Thanks for your help and patience! ✌️

@splitbrain splitbrain merged commit c01a028 into dokuwiki:master May 31, 2020
splitbrain added a commit that referenced this pull request Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants