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

Add a 'returnonly' option to all xhtml link types #1239

Merged
merged 1 commit into from Jul 18, 2015

Conversation

andyboeh
Copy link

This PR adds a returnonly option to all xhtml link types, not just to internallink. In order to support different renderers in the data plugin, this is required.

@scrutinizer-notifier
Copy link

The inspection completed: 2 updated code elements

@Klap-in
Copy link
Collaborator

Klap-in commented Jul 15, 2015

Implements an significant point of an issue in old bug tracker as well:
https://bugs.dokuwiki.org/index.php?do=details&task_id=1549

@selfthinker
Copy link
Collaborator

I'm generally very much in favour of this. But...

I just wanted to write that these changes should be consistent with the core code but then discovered that we are not very consistent to begin with. I think this is what we mostly do => https://github.com/splitbrain/dokuwiki/blob/master/inc/template.php#L465-472 (i.e. variable is called $return and what should be returned or printed is first stored in a variable) <= but I also found lots of other cases where we do it differently...

@andyboeh
Copy link
Author

Well, I used the variable name from the (already existing) internallink function and tried to copy its behaviour. While doing so, I noticed the same, i.e. that the variable is simply called $return in another function or that the handling is slightly different.
However, $returnonly makes more sense to me as it implies that nothing else is done apart from returning the rendered code.

@Klap-in
Copy link
Collaborator

Klap-in commented Jul 16, 2015

Which approach is preferred?

@splitbrain
Copy link
Collaborator

I think it's okay this way.

splitbrain added a commit that referenced this pull request Jul 18, 2015
Add a 'returnonly' option to all xhtml link types
@splitbrain splitbrain merged commit b56e271 into dokuwiki:master Jul 18, 2015
@furun
Copy link
Contributor

furun commented Jul 19, 2015

Maybe a way would be if new functions would be defined, witch return the data and not use print/echo at all. And the old function can be used as a "wrapper" with the only function to print the data. In this way the functions can be deprecated, and anyway keep functional with plugins and old code.
Code in dw then don't need a other extra parameter like $returnonly, but would defined a list of new functions. And the new functions could be used more freely and intuitifly.

I don't know what makes more sense, parameter-$returnonly vs new-functions.(?)

@furun
Copy link
Contributor

furun commented Jul 19, 2015

((Out off topic:

i think since long time, if it would be possible to use a automatic patching system for deprecated functions or renamed/parameter-reordered functions. (i seen this by the software unity3d, the only software i know from, witch updates codes of costumers automatically.)
Don't know if such a system exists in a php project some where. It could reduce the inhibition for code optimizations in general, but could cause many problems too.))

@selfthinker
Copy link
Collaborator

There are lots of functions which would need to be deprecated then.
If we're ever going to do something like this, I would also prefer to change most (at least optional) parameters to an array. Some parameter lists get ridiculously long, many of which are rarely used.

@furun
Copy link
Contributor

furun commented Jul 19, 2015

Yes i seen this too and would prefer a array for the (at least optional) parameters. The large list of parameters is the reason why i put the idea here to use new functions, instead of a other parameter after a chain of already existing ones. (But maybe this would cause a to big rebuild of existing codes. (?) ) At least dw could use them, and the templates/plugins had do be updated, probably over many years. Or the old ones could be keeped for convenience.
...but it would be a bit of work...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants