Posted by chx on March 14, 2009 at 4:55pm
20 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | filter.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Issue Summary
This is a direct offspring of the help patch. The code is taken from there but changed the prefix to internal: and the strtr to str_replace. Once this is in we can use this in the help files and everywhere else and we get caching for free.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| internal_filter.patch | 1.35 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
Comments
#1
#2
First I uploaded the wrong patch, then left in debug. now...
#3
DamZ does not like node/123. Sure, I can copy the foo/bar from the test.
#4
Now, I forgot to list the poor thing. No wonder, given that op 'list' was not properly formatted.
#5
Code looks nice. This is the result on the node edit form if you add the filter to the default text format:
#6
Now with a less confusing help tip.
#7
yay, pathfilter in core :) but...
1. how is url('$1') supposed to find a path alias for anything other than literally '$1'?
2. maybe an edge case given they will usually be individually quoted, but the preg_replace does a greedy match so "[internal:node/1] [internal:node/2]" won't work as you expect.
#8
This should take care about John's concerns:
- Extended the test
- Fixed the filter (we really need to use the /e modifier, because we need to properly alias paths).
#9
And don't forget to remove old cruft ;)
#10
Comments from #drupal:
* testInternalFilter has no PHPDoc.
* if we're going to call _filter_internal() from the test, should we drop the leading underscore?
* $f is not an acceptable variable name.
* also, what is _filter_internal_callback? Looks unused.
#11
...was what I said about #8! ;)
#12
AHEM. code needs review?
#13
And from #10. The test is an unit test, so it's logical it tests internal functions ;)
#14
Reworked the wording of the description with catch via #drupal.
#15
#16
So, after talking with webchick on IRC and getting the rundown I'll +1 this. I prefer the internal: term over base_url and I really like the fact that the syntax can be used in other places in core rather than being "just" a help system thingy. That will make explanation and use more consistent for folks.
#17
Ok, then!
Added a CHANGELOG.txt entry and committed. Thanks, folks!
#18
Just tagging, so I can keep track.
#19
Could we include a [files:] filter too? and automatic conversion? a.k.a. can we get a fully fixed PathFilter in core?
shameless plug: #402296: Call for Action: Get Pathfilter Additional Functionality in before D7 makes pathfilter obsolete (Pathfilter in Core???)
#20
We talked about doing automatic conversion of any link that starts with "/" to use internal. This is problematic for the following reasons:
a) Sites may be using other programs apart from Drupal on their domains (amazing, but true! ;)). For example, you would *not* want a link to "/phpMyAdmin" to get translated to http://www.example.com/drupal/phpMyAdmin.
b) Without a special token that can be regexed against, we now need to check *every* a, img, link, style, script, and who knows what other tags. That seems like a big performance drain.
As for [files], I'm not really opposed. But we don't (afaik) need it for help, which was the point of committing this patch. So if you want to champion for that, it should be done in another issue.
#21
Until now, Drupal core did not implement inline macro tags. Now it does. And it introduces (and thereby teaches) a syntax that is known to be inflexible.
Furthermore, did anyone test whether this works with any client-side editor? ...
#22
Sun: could you elaborate on your concerns?
#23
Recommended reading: #80170: Inline API (dates back to 2006/Drupal 4.7)
Specifically about syntax: #32838: New Inline Filter code
Why is this inflexible? Because the syntax is known to conflict with macro arguments.
What would be a proper macro syntax?
[link|path=admin/user/permissions|title=Administer permissions|absolute=0|fragment=my-anchor]What would be an alternative syntax?
url:///admin/user/permissionsor similar. No square brackets.Testing what has been added to core requires to install Wysiwyg API with all supported editors, use the "Link" button of each editor and enter this macro tag. Afterwards, disable the editor and ensure that a) the square brackets are not escaped b) the entire href attribute value does not have a leading slash or any other addition or alteration.
Also:
Why is the filter and function called "internal" and not "url"?
Why is a filter tag called "internal" not intended to display a piece of content for internal users? What has "internal" in common with links and URLs? Ah, yes, Drupalitis.
Since when does url() support only internal paths?
Why is this a private function?
What happens if the user disables this filter?
What happens if users "suddenly" start to use this macro in their content and we suddenly decide to rename it in Drupal 8?
...
Anyway. Let's just continue to break each and everything.
#24
Please do NOT re-close this. Your concerns are valuable and needs discussion.
#25
Well, then we need something more sophisticated, I agreed with sun that a) he stops using macro because that leads to an endless bikeshedding over what the fuck is a macro b) we remove the [ ] and just let <a href="internal:foo/bar"> happen.
#26
Please revert this and let others get in inline API or token (which is already beyond saving -- I tried and was chased from the issue) in core.
#27
+1
#28
#29
@sun: From your point of view, would there be a way to turn this "shameful" patch into a not so shameful one, e.g. by replacing the 'internal' tag by
or would that have to be a sweeping change as in e.g. moving Inline API to core?
#30
I don't think that last comment meant to change the status.
#31
Rolled back. I agree this needs more discussion. From sun on IRC:
"There is a difference between auto-formatting filters and macro filters. The former is what we have in core thus far: Filters that don't require you to enter something special, they just convert content based on standards (not defined by ourselves). Macro filters are different: The require a very certain syntax, most of them can take parameters, and when their syntax changes, all content that used them in the past *are broken*. My goal to solve the latter is to get Inline API into a core-worthy state, which would (also) handle inline tag /upgrades/."
So essentially, we're introducing a special syntax here, and if we ever change our minds that's a whole lot of hassle.
Tokens in core is likely the way to go. Then this would be one standard way of introducing "special things" everywhere. sun, feel like un-sticking another issue? ;)
I'm also curious to know more about inline API and how that would work. Is that basically to allow each site to define its own inline styles?
#32
Removing tags to avoid confusion,
#33
#34
We should probably extend Extensible HyperText Markup Language. So use standard XML syntax (< > instead of [ ] brackets, tags, parameters). These are the benefits:
1. Set of powerful and efficient PHP functions for XML, XHTML, DOM processing. Some of them are part of the PHP core and are enabled by default. See http://php.net/manual/en/refs.xml.php
2. Standard escape entities for < > brackets.
3. No need to learn completely new syntax. Standard parameters available.
For new tags we should probably use prefix "drupal:" for Drupal namespace, for example:
<drupal:title /> <!-- Display node title in body text --><drupal:authorName /> <!-- Display user's name -->
For internal links we should probably "extend" Uniform Resource Locator with new scheme, for example:
<a href="drupal://node/1" /><a href="internal://node/1" />
More examples: http://en.wikipedia.org/wiki/URI_scheme#Official_IANA-registered_schemes
Would be great if we also consider connection with Node reference and User reference modules and RDF too.
#35
@mki: Sorry, but a big no to any DTD customizations. Client-side editors (aka. WYSIWYG) don't support that and will simply strip anything unknown from the content.
We don't need to invent a new URL scheme, because if we go with a custom scheme, then the URLs will be converted to a standard scheme by the filter before the content is displayed.
However, also a custom URL scheme would have to be tested for compatibility with client-side editors first.
#36
Here's a patch, since we need to remove this from CHANGELOG.txt if it is not going into Drupal 7.0.
#37
Ignore that last patch. ;)
#38
Please commit and reset to active.
#39
D'oh! Thanks. Committed the reverting of CHANGELOG.txt.
#40
Just as an aside, have the main search engines fixed their issues with using cached pages BASE href's? The _base_href attribute used on cached Google pages seems to work fine. If so, are there any outstanding issues now with using the BASE header tag and relative URL's? Yep, back to the good old days of Drupal 4.6!
We've used the BASE tag on a couple Drupal 6 sites recently due to the issue of getting the users to correctly use the internal: tag on links and we have yet to find any issues.
#41
We're talking about doing this as a filter whose output will be cached, right? If so, then it only has to be done when the node is edited. No biggie, right?
I too am not a big fan of the idea of introducing a special syntax or prefix of some sort to paths to get this to work. As my module Pathologic demonstrates, it's possible to determine the user's intent with just some fairly simple heuristics and maybe a
file_exists()(yes, I know, but again, it only has to happen once). Check out the code, or if the admittedly undercommented regex strings make your mind boggle, check out the "Is configuration necessary?" section of the docs page for a plain-English explanation of how it works.This feature in core? A good but perhaps unnecessary idea. Introducing nonstandard markup in order to do it? No thanks!
(But if it has to be done with a tag of some sort, may I suggest a comment-style tag like
<!--internal-->? It should break fewer things and has precedent (quality of that precedent aside) in the form of<!--break-->.)#42
@Garrett Albright: Please ping me when you are in IRC. We need to talk.
#43
I've recently taken over project ownership of Path Filter (which uses the internal:node/nid syntax) and am keen to see this functionality make its way into core. Work is also taking place to extend Path Filter to support a files: syntax, which was also mentioned earlier in this thread (#127484)
I have also begun communicating with Garrett Albright about the possibility of merging Path Filter and Pathologic, for obvious reasons - they do essentially the same thing, they just go about it in a slightly different way (and Pathologic actually does a lot more). With almost 1000 people using each of these modules and a push to get this functionality into core, if we are to merge these modules (and possibly if we don't too) then it would make sense that the module(s) were headed in the same direction this core patch - if indeed this core patch is going anywhere!
I am in two minds about weather or not special syntax should be applied ([] <> internal: etc.). Obviously, this is the way Path Filter works, and I'm used to this, but that certainly doesn't make it right.
Anyway... just though I'dd add myself to the discussion.
#44
For reference, here is another discussion from the PathFilter issue queue, on the issue of using internal: (or another prefix) or not: #109992: Replace regular internal paths with URL aliases
#45
Let's solve this issue: we have proposals for:
1. internal://node/123
2. wiki - like syntax. [link:node/123] or [link:node/123|title]
let the discussions begin.
#46
I think we pretty much agree on that internal:/// is the least intrusive.
#47
On irc an example of "internal:///drupal.org/node/401956#comment-1803458" was used and to me, I see no difference in using such a link from using "http://drupal.org/node/401956#comment-1803458" except that end users will know how to use the latter by default.
#48
Most probably, a solution like http://drupal.org/project/pathologic will be the best way to move forward. I already started with a #462870: Code clean-up, but the maintainer seems to be unresponsive.
#50
It's true that the maintainer (me) hasn't worked on it in a while, but the dev release does include the patch you submitted. :P But your bump plus a support mail question has got me taking another look at the code tonight - perhaps I'll be making another beta release tonight.
I think that no prefix at all is the least intrusive!
I'll go hang out in #drupal tonight if anyone feels the urge to ping me about this.
#51
#52
Can we get input from UX people or trainers with experience in teaching people? There are technical hurdles to overcome but one of the biggest is for everyday users. Non-technical content contributors. If non-technical users are inputing internal://node/123 or [link:node/123|title] which will be the easiest for them?
#53
As @webchick suggested in #31
> Tokens in core is likely the way to go....
Not exactly the same, but related, using tokens to replace paths, links #1445144: Build better tokens with parameters (and use them to replace help text)