I'm creating wiki-like features on my site so that if someone with the permission to create new pages goes to a 'page not found' page then it gives them an option to create a page at that address (i.e. at that path, so if they goto /somepath they can easily create a page with path /somepath).

To do this its very handy for the 'path alias' field on the new node page to be pre-filled with a value using e.g. node/add/page?edit[path]=somepath in the same way that you can prefill the title field using node/add/page?edit[title]=sometitle

I've got this to work by adding the following line to path.module at line 211 as thus:

      case 'form pre':
        if(!$node->path AND isset($_GET['edit']['path'])) $node->path = $_GET['edit']['path'];
        $output = ...

... my line is the 'if' line in the middle above.

Could this be added to core? I'm sure others could find it handy and it would save me from having to rewrite the line on every upgrade. If it can't be added to core, is there anyway for me to put this line into my own custom module somehow so that I don't have to rewrite it every time I upgrade?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfaocle’s picture

Status: Needs review » Active

Not a patch, but I quite like the idea.

pfaocle’s picture

Version: 4.6.0 » x.y.z
Status: Active » Needs review
FileSize
956 bytes

Here you go, ready for voting.

Bèr Kessels’s picture

Hmm. I am not sure. In this way, it can only be used by the wiki module. And IMO it is a bad thing to facilitate single implementations in core.

In other words: I would definitely +1 this were a more general solution. So, what about a hook? IMNSHO that is actually over the top, a hook, but it would help other modules that want to fiddle with the path too.

jakeg’s picture

It shouldn't be implemented as a hook, that is overkill. Apart from wiki modules, you may generally want to be able to submit a node with any number of the fields prefilled (i.e. not just 'path' but other fields as well... though I can't think of an example right now). Having lines such as the one I suggest for those fields is the only way to make this possible without individuals having to hack the core and update their hacks each time there's a new release.

ezheidtmann’s picture

jakeg, an example is book module, where the "add child page" link uses its own URL syntax to prefill the parent field. If a hook for listing prefillable fields existed, the URL syntax could be made consistent with the rest of core and contrib.

Bèr Kessels’s picture

an overkill indeed.
but $node->path = $_GET['edit']['path'] just doesn't feel right. So I was thinking aloud about how to solve this. Anyone?

ezheidtmann’s picture

FileSize
1023 bytes

Why not just prefill $node with everything in $_GET['edit']? This patch should do that (untested)

eafarris’s picture

/me gets errors with your patch, but here's a tested version. I really like this idea; without it, I can't get a bookmarklet to fill in the 'url' field of the weblink node type, for example.

eafarris’s picture

Component: path.module » node.module
FileSize
665 bytes

wrapping that section in an if ($_GET['edit']) block gets rid of the array_keys() error on regular posting pages. updated patch attached.

Also, moved issue to the node.module.

Bèr Kessels’s picture

I prefer that last patch. But, still, I dislike it.

As far as I can tell, you can achieve all this with properly invoked nodeapi calls.

eafarris’s picture

Bèr,

Do you mean that each module should implement its own nodeapi for converting $_GETs to $edits? Seems like it would be better to have node.module do this for everyone, or am I misunderstanding you?

Also, I'm wondering what security ramifications a patch like mine would have. Drupal's pretty good at checking input, but you never know.

Bèr Kessels’s picture

What i mean is that with nodeapi, you can add the following to *wiki.module* :

wiki_nodeapi() {
  //...
    global $node;
  //....
    case 'form pre':
      node->path = wiki_get_path();
  //...
}

That way you leave the hard work for the module that needs the hard work done.

And security: I am simply not sure. But setting the $node object from *_GET data just does not feel right. Off course this should be no problem, because anyone can alter the $POST too. Its just that I prefer to leave it to code to set the node object, and not to url hacking.

eafarris’s picture

Ok, I see.

While that would solve the original poster's issue, I have a different need that's solved by this patch: bookmarklets and their ilk. With Moshe's bookmarklet ( http://www.tejasa.com/bookmarklet ) I can post to a blog entry, but hacking it to fill in, in my example, the 'url' field of weblink.module requires this patch for it to be passed along to the form. This also opens up possibilities for a Greasemonkey script I'm working on, too.

Without the patch, only title, teaser, and body are passed along from the URL. That's quite constraining, all sorts of cool stuff can't happen because the fields are simply thrown away.

I'm not after module->module or module->node interactions, I'm looking at filling Drupal forms from outside.

Bèr Kessels’s picture

Title: Prefill 'path' field on new node form » Prefill the $node object from the $_GET variables
ezheidtmann’s picture

I agree with eafarris - it's better to have node.module do the work for everyone rather than each module having its own code to fill fields on the form page.

I too was worried about the security ramifications. But I thought about it for a while and now I believe there's really nothing to worry about. The changes only apply to node/add pages, so no data can get from $_GET to the database without the user's approval. As long as the data is properly escaped, it can't mess up the HTML on the page.

Modules which use hidden fields may behave strangely if a hidden field is filled from $_GET, but those are certainly in the minority and can avoid the problems simply by checking $_GET before using a value from $node in pre-filling the form.

nevets’s picture

Why not store the fields in node under get so instead of node->somefield it would be node->get->somefield?
At least that way there is only one node field that modules need to care about avoiding and it reduces the likelyhood that the change will impact existing modules.

ezheidtmann’s picture

nevets, having the values in $node->get is no different than having them in $_GET['edit']. Modules would still have to to be modified to use the values to prefill the form. This patch allows fields to be prefilled on node/add pages without modifications to node or nodeapi modules.

Boris Mann’s picture

*bump*

Let's look at this again -- it enables all sorts of RESTful interaction with Drupal's node creation, especially bookmarklets for creating non-standard node types -- weblink, event, etc.

eafarris’s picture

Offset changed in HEAD, here's an updated patch.

eafarris’s picture

offset changed again in HEAD, updated patch.

*bump*

eafarris’s picture

Same patch as before, in diff -u format.

chx’s picture

Status: Needs review » Closed (won't fix)

That's why we have nodeapi. For 4.7 , I'd look at op 'prepare'.

eafarris’s picture

So, instead of the node.module offering this as a service, globally, each content module would have to implement it themselves?!? sub-optimal.

chx’s picture

no. you write a rest.module and prefill for other modules.

Boris Mann’s picture

Status: Closed (won't fix) » Needs review

I'm bumping this again -- eafarris is commited to maintaining it, see http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/eafarris/pref...

This does enable extra functionality that is very useful for bookmarklet features and other inter-node operations where you're using the URL to pass items. This seems a clear case where a small change will immediately enable interesting remote interaction across all node-based modules.

Other than Ber and chx disliking it, what does this harm? And really, who is allowed to set to "won't fix"?

Bèr Kessels’s picture

I do not dislike the feature, I disliked the route taken. So take this as another +1 on the last patch.

That said, its certainly better then nothing, so ill shut up, after giving another cool user-case :)

Dru.palituc.us: Drupal, freetagging, links module, tagadelic, a bookmarklet, example.com/?url=foobar.com links
... and this patch. And viola, you run your own delicions killer.

Note that i tested the last patch, it worked and the said bookmarklet (and incoming example.com/?url=foobar.com) features worked.

On a sidenote, should we not do some input sanitising, though? Like making sure the variables are not longer then foo, and do not contain certain characters, like HTML code?

Bèr

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
631 bytes

*shrug* if I take out personal considerations and take only http://drupal.org/patch/review into consideration then this is ready to go. I even rerolled it so it's a proper patch.

Boris Mann’s picture

Thanks Ber and chx.

Ber: it just does a prefill, which I believe won't do any harm, since it will get filtered on node submit. Theoretically, the links are generated by drupal module code elsewhere that might do such filtering.

eafarris’s picture

Bér: I think that while some sanitizing may make us feel better about the functionality, Boris is right that this is only prefilling the fields, and, even if you could push the submit button from the URL, things will still be filtered and checked at that time. As long as we can be confident that this cannot be exploited to go around the normal submit procedure, I don't think there's any harm here. I can't see any way to get around that, but, I can't see a lot of things.

chx: Thank you for taking a second look at this. Making a rest.module out of a five-line patch just doesn't feel right to me, especially since the current system is hard-coding three rather abitrary fields. This is something that node.module should have been doing all along.

Boris: I appreciate your efforts here.

chx’s picture

Status: Reviewed & tested by the community » Needs work

I was too fast in setting this RTC, I should have thought the security part over.

Is there a security risk? I am not sure this is even security problem. With form API in 4.7 , one expects that node table values are sane -- you can't set a format you have no access to, if I define a radios with 0, 1 values, then I'll get 0, 1 and nothing else and so on. With this patch, you can sneak in insane values to $node. It's not likely to get into the database without running through the form API but this CAN introduce very sneaky bugs, even security bugs IF a contrib module saves the bare $node object, which it believes to be sane because of form API. It may even add something like a preview, ie. a textarea filtered using the filter that comes from the database which it thinks it's safe. Yes, it's possible that it'll never happen. But this prefilling stuff is scary.

If you can quell these fears, then let's go further.

eaton’s picture

Perhaps we can introduce (another) optional flag on formapi elements. pre-populatable? something like that? the protected system-generated fields like filter format and author can be 'locked' against pre-populaton no matter what get variables are passed in?

make it opt-in rather than opt-out, and I think it could work well. It would require that third-party modules be updated, but nothing would break.

chx’s picture

Sure! The most voodoo-like code is the one that walks the $edit tree in form builder. Now, you want to repeat that to walk $_GET['edit'] and the juggle with ($getted, $posted, $default_value) ? Sounds like _fun_ !

Do not get me wrong, I can write it but are we sure we want to make form API is even more complex??

chx’s picture

I came to this conclusion: I'd rather see a rest.module walk the (node) form tree and overwrite #default_value s than me doing the same in form.inc when most of the time drupal does not use REST .

This is not the elegant five line patch you suggest. Changing $node is not a good idea.

chx’s picture

I am not setting won't fix because if someone comes up with a nice & elegant way to change form_builder to use both $_GET and $_POST then I'll be happy. I guess that's post 4.7.

Bèr Kessels’s picture

okay. so we are back where we were. Chx states, that if we are going to fill anything with anything, that is just asking for trouble (re: "it just doesnt feel right", above).

My solution above, was to let those modules that are in charge, and that know what var in $node->$var is going to be populate them. That is certainly the safest route.

So, without going back and forth all the time: how to solve this:
* Just filling $node->$_GET is unsafe. (the current solution)
* Maintaining a list of allowed filed vars (is now in core) is too limiting
* Maintaining a list of disallowed vars is unsafe. That is opt-in security.
* Leaving this to contrib modules is too much work

chx’s picture

As said, if you write a mdoule which walks the form tree and change the #default_value s then you are OK. This does not need to be done in contrib moduleS just one contrib module. This I feel safe because $node is not changed. And also this solution would apply to more than node forms.

Dries’s picture

The code comment above the changed bulk should probably be updated/extended to the new situation.
I think it should be secure, if the forms API does it job. I'd tripple check though.

eaton’s picture

Dries, I think chx's concerns are based on this:

previously, all modules had to manually validate data sitting on $node lest they expose themselves to security risks.
In the new formapi, anything on $node can be counted on to be scrubbed clean, fresh from the database. Values are passed through form edit values and validated THERE, then pushed to node and saved.
This bypasses that practice, meaning that modules can no longer trust that $node data has passed any kind of validation. If there is custom form-building logic based on those values, or what not, it could cause problems.

I'm struggling a bit to think of a scenerio in which something genuinely dire could come of it, but chx is correct in that it violates a key principle of the new API: $node is sacred, the edit array is where you alter stuff. Perhaps an opt-in flag on form elements, the way we handle #collapsible, combined with a module that loops through the form and populates the #default_value of any #populatable fields with correct _GET[] values? That would make sure that the values are still going through the form API even before submit.

eafarris’s picture

Status: Needs work » Closed (won't fix)

prepopulate.module now exists for this purpose. It needs work, but it exists.

http://drupal.org/project/prepopulate

killes@www.drop.org’s picture

is somebody keeping track of those changes? Might be handy for a future release announcement. I recall that some people were pretty surprised that their all-imporant taxonomy block was gone about a year ago. :p

eafarris’s picture

Attached is a patch for CHANGELOG.txt documenting this. Reword as appropriate.

killes@www.drop.org’s picture

Status: Closed (won't fix) » Fixed

committed.

I wonder if anybody will read this. :p

Anonymous’s picture

Status: Fixed » Closed (fixed)