Specifying the resource format via a URL extension (like "node/1.json") no longer works in Drupal 7.37

Confirmed. If you access a URL like http://example.com/node/1.json this used to return node 1 in JSON format, but with Drupal 7.37 it returns a 404.

I imagine other methods for retrieving the entity in a particular format (like setting an 'Accept' header) still work, but haven't checked.

Apparently this feature never worked in PostgreSQL (#1565952: Request to node/1.json Returns 500 Service unavailable) but did in MySQL.

It seems to have been relying on the fact that on MySQL databases, calling node_load('1.json') would return the node in Drupal 7.36 and earlier. This bug was fixed in Drupal core as a side effect of #1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID.

I will add a link to this issue in the Drupal 7 release notes.

A possible idea for fixing this, could the module implement hook_url_inbound_alter() to strip the format (i.e. "json") off the end of the URL and thereby tell the rest of Drupal to treat the current path as if it's the regular node/1?

When stripping the format off the URL in that hook, it could also save it somewhere so the rest of this module (i.e. the page callback) can access it and use it as needed.

@David_Rothstein that's unfortunate, but wouldve been hard to catch in that previous issue as well. At least whatever the fix is will fix both mysql and other DB systems now :)

Maybe the way to prevent some of these sort of breakages is to run automated tests against the most-used modules for 7.x and then also for 7.(x-1) and compare?

@David_Rothstein that's unfortunate, but wouldve been hard to catch in that previous issue as well. At least whatever the fix is will fix both mysql and other DB systems now :)

Maybe the way to prevent some of these sort of breakages is to run automated tests against the most-used modules for 7.x and then also for 7.(x-1) and compare?

Using the node/1.json in a browser does not work as you mentioned.

Performing the request with the Accept: application/json header (and optionally the Content-type header) *does* work.

Hi, see my comment on the core issue.

@Koen.Pasman, your report of not using .json but instead just setting the accept header does work, but it returns a 301. When I dug through the code, I found this reason (default switch option in restws_page_callback):

Consumers should not use this URL if page caching is enabled. Drupal's page cache IDs are only determined by URL path, so this could poison the HTML page cache. A browser request to /node/1 could suddenly return JSON if the cache was primed with this RESTWS response.

So while just setting the accept header works, it means we can enable caching. This doesn't seem like a good permanent solution.

Ah, well spotted! So i guess I need to roll back to 7.36 in order to fix this until a patch/fix is available.

Grrr, this is very unfortunate and slipped through because at this point I don't have RESTWS running on any site I maintain anymore.

So this is a good point to announce that I'm seeking a new RESTWS maintainer! Please send your applications in form of a patch to this issue :-)

Not sure if hook_url_inbound_alter() is a good solution, but I also don't have a good alternative suggestion. I would probably move the RESTWS paths to /node/1/json in a hook_menu() implementation, but that of course breaks all clients that rely on /node/1.json now.

Here's a very temporary workaround to this using hook_url_inbound_alter():

/**
 * Implements hook_url_inbound_alter().
 */
function restws_url_inbound_alter(&$path, $original_path, $path_language) {
  if (strpos($path, '.json') !== FALSE) {
    $path = str_replace('.json', '', $path);
  }
}

This results in a 301 redirect to same URL without the format attached to it. This happens each time the URL is requested, so it doubles the URL requests your application makes.

I agree with @klausi that moving the format to another URI segment is the best path, but hopefully something like my example code will tide us over.

ok, here's the >temporary< fix from @pianomansam for those running 7.37 and restws, this temporary patch will not work with older versions of D7 and we cannot guarantee that it would work with newer versions.

Hoping someone finds a better fix. For now this might get the job done and I haven't tested it.

filename correction

Here's a patch that attempts to fix this:
- Can access resources as node/1/json (also works with xml and rdf)
- Works with page caching enabled
- Improves negotiation of the MIME type requested in the Accept HTTP header
- restws.module now is compliant with Drupal Code Standards and PHPDepend complexity thresholds. Ie., restws_menu_alter() has been split in two aux. functions. Most notably restws_page_callback() has been restructured to incorporate a cleaner logic.

I believe consumers/clients wanting to upgrade to Drupal 7.37 should implement URL rewrites at the Apache/Varnish/Nginx level, and/or modify their client code to access resources with a slash (node/1/json) instead of a dot (node/1.json). A warning should be added to the project page in a visible place noting this, ideally with the .htaccess rules or Varnish VCL code.

I ran some smoke tests but haven't been able to run the module's tests. I expect the drupal bot to run those tests in a short while after this post.

Re-rolled patch attached.

3rd. version of the patch. empty() does not work as good in older PHP versions.

Sooo... if d.o upgrades to core 7.37 it will break all of drupal.org's api?
See https://www.drupal.org/api about them.
like https://www.drupal.org/api-d7/comment.json?node=1978202

That specific one may still be OK as it has the node ID in the query parameter (without .json appended to it)

Not the whole drupal.org API will break, but all URLs that go directly to single entities like the one for this issue https://www.drupal.org/api-d7/node/2484829.json will break.

This problem seems to affect the queries as well.

I ran a test on 7.35 for node.json?type=article, restws 2.4, and it worked fine.

Then ran the same website with core updated to 7.37 (same restws 2.4) and it's returning 404.

I had to rewrite the routing logic of the patch, and in so doing I found the dots are working again.

I think the 404 is not thrown by core, but by line 162 of the module, because of some bug in the current routing.

A brief update on this, since I'm not getting the time to continue working on the patch.

It seems that, through alteration of the menu system, Drupal can be tricked into NOT loading the entity and let something like 43.json be handled by restws. However, the attempts I made did not pass the test suite successfully.

So, I can see 2 options (not mutually exclusive):
1. Further explore the menu alteration "trick"
2. Complete the switch to a /node/43/json URL format (and document any change in the web server side if needed).

1) would be ideal, as it would a require minimum effort
2) seems to be a safer/predictable choice

Hi, to me also is not working when i use the HTTP methods DELETE or UPDATE, responses status 200 home page.

re #25; I'd support modifying the style to be more of the form node/id/format instead of node/id.format and node/format instead of node.format for the resource query calls.

This is more in keeping w/ the rest of core / contrib as i've never seen anything else w/ the .json (incredibly useful as it is) style calls.

As #2 suggests, legacy calls could also still be supported by invoking hook_url_inbound_alter (https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func... ) which could detect .json / .xml / .rdf paths and rewrite them to /json /xml /rdf based paths. By shifting to the new path structure though I think it would avoid some of the issues associated w/ #2 outright and overall be less hacky.

What about trying a hook_menu_item_alter() that would change the load_functions to a restws method that would use entity_load()? It's a little gnarly, but for the 'node/%node' paths, something like:

function restws_menu_get_item_alter(&$router_item, $path, $original_map) {
  $resources = &drupal_static(__FUNCTION__);
  if(!isset($resources)) {
    $info = array_keys(restws_get_resource_info());
    foreach ($info as $resource) {
      $resources[] = $resource . '/%';
    }
  }
  // Check if $router_item path needs an updated loader.
  if (in_array($router_item['path'], $resources)) {
    $router_item['load_functions'] = serialize(array(1 => 'restws_resource_load'));
  }
}

function restws_resource_load($identifier) {
  $dot = strpos($identifier, '.');
  if ($dot > 0) {
    $identifier = substr($identifier, 0, $dot);
  }

  // Discover the entity type.
  $path = $_GET['q'];
  $original_map = arg(NULL, $path);
  
  $entities = entity_load($original_map[0], array($identifier));
  return !empty($entities[$identifier]) ? $entities[$identifier] : FALSE;
}

@ashepherd - Thanks.. this works for me. Attaching a patch.

I think we should follow node/%nid/json format as it is more drupal like. But we should also try and protect the already existing format along with the new one and slowly fade it away in a new major version release.

Thanks

Tried patch on simplytest.me and it worked.

Agreed it would be nice to adopt the /json as well, though this patch works; I wonder what the performance implication / overhead of utilizing this patch is since it fires on every node loaded, especially in like a book / outline of nodes which would triggere this per node (I think).

as per $dot = strpos($identifier, '.'); if we don't find a . can we bail early?

We can move this line up when we are checking if the router_item['path'] needs to be updated or not. But that would mean that paths without the '.' will be attached to normal function and paths with '.' will be attached to new function.

Attaching same function for both paths with and without a '.' will help keep everything in one place.

Performance wise, I don't think this patch is a big overhead as the most resource intensive method 'entity_load' will be called either ways (by restws_resource_load or by node_load).

But at the same time node_load supports $conditions and it might be a better idea to use core module functions rather than trying to replicate their functionality for consistency.

The latest patch unfortunately causes views pages to crash.
To reproduce:
- apply patch
- create view (with content) page with path
- go to created page -> *BOOM* ;-)

The problem is this tries to do:

$entities = entity_load($original_map[0], array($identifier));

With the first parameter the view path, which isn't a valid entity type.
I've added a check to see if the url maps to a valid entity type before trying to load the entity.

Sorry, this does a wrong check.
This should be better. And should be a better patch;

I wonder how that happened as we are only attaching this function for the REST-able resources. Is views a supported resource type in RESTWS??

Ok, lets try to make a better patch. (Note to self stop trying to edit patch files by hand).
Only change is a check on the entity load method.

  $entity_types = entity_get_info();
  if (array_key_exists($original_map[0], $entity_types)) {
    $entities = entity_load($original_map[0], array($identifier));
  }

It seems this call get's called on views of 'Rest-able' resources. And there it uses the path, which could be anything. (and if not a valid entity type it crashes on the entity_load).

@mallezie, I'm not getting your patch to work when sending a PUT to update a node.

Invalid data value given. Be sure it matches the required data type and format. in EntityDrupalWrapper->set() (line 738 of sites/all/modules/contrib/entity/includes/entity.wrapper.inc

I couldnt get the patch to apply so I re-rolled it, I'm only doing GETs here so I'm unsure about comment in #38

I'm seeing that the path is invalid when I try to add it to a node

restws_resource_load() is returning FALSE, in the patch "$path = $_GET['q'];" is in this case the path to "admin/structure/menu/manage/main-menu/add" not the path that is passed to it (node/176 for example)

Attached is a patch that only works with node entities, to illustrate the point it seems like $identifier never knows the entity_type only the Id, this patch got us out of the mud for now

The approach in #43 makes sense to me. I tested in my environment and everything I was expecting worked, however I too only make use of the GET scenario.

@pianomansam, perhaps you can test this patch? This is just @bangpound's work in patch form.

I checked the failed messages.
RestWS module tests are all passed.
The failed tests are in RestBasicAuth module. user/1.json response is 404 whereas 200 is expected. I manually tested the user/1.json string on my machine as well as simplytest.me and it works alright.

Submitting a retest request.

Thanks

Hmmm.. Failed again.. not sure what's going on there... but the patch works and the tests are passed for the main RestWS module... I will buy it :)

would it make sense to wrap this in a simplier regex to see if there's a call that even has . in it? Otherwise the proposed patch at least loads all resource and format info and applied the preg replace, regardless of if it ever should. This (while marginally) will impact every page load via processing when we could be limiting it to just those calls utilizing the . in the uri

same results as previous version of patch, seems to bork basic auth. Going to fire this up in Vagrant and see if I can debug w/ some known working restWS communications

ok, pushed this patch along w/ latest of core into my vagrant instance which every page calls other pages (at some level) for network responses. After applying this patch I can confirm that all the calls still work and I make heavy use of both the node.json? query style as well as the node/12.json request style.

I tested PUT statements as well and they are still working BUT I am also using the Header / content encoding method stated above that it still works this way.

I don't issue PUT statements against the node/1.json style paths and from my own documentation I don't know that I could ever get this working without passing through the content encoding method.

I don't query the user resource (ever) so I can't confirm if the errors thrown by the test bot are valid or not

Any more confirmations?

No I wasn't able to reproduce any issues using this approach. The patch isn't causing the error though as I feel this error is thrown w. an empty patch. Looking at restws.test's httpReqest function it appears to have more flexibility then the one utilized in restws_basic_auth.test.

I do GET, POST and PUT functions in my restws setup all the time (and tested all of them w/ latest core and this patch) without issue but I don't currently do GET's against the user object, though I'm not sure why that would differ in any way from the rest of the resources.

If this solution is accepted, I agree that the errors are not part of this patch and that Basic Auth just has issues.

Yeah, is there any way to verify that though? Maybe like submitting a dummy patch or something? I'm running this in production now for a few days after lots of testing ahead of time and not noticing any ill effects of the 7.37+ break.

The exceptions are new. I wonder if something like this is needed:

<?php
@@ -206,10 +206,10 @@ function restws_init() {
   $formats = array_keys(restws_get_format_info());

   $pattern = '/^((?:';
-  $pattern .= implode($menu_paths,'|');
+  $pattern .= preg_quote(implode($menu_paths,'|'), "/");
   $pattern .= ')\/[1-9][0-9]*)\.(?:';

-  $pattern .= implode($formats,'|');
+  $pattern .= preg_quote(implode($formats,'|'), "/");
   $pattern .= ')$/i';
?>

I tried this exact change and it didn't work. But something is breaking in the preg_replace().

Are the 3 remaining errors also in the empty patch? If so, then this is ready?

The 3 remaining tests pass if I change user/1.json to user/1/json.

The routing for user paths isn't using the substituted page callback.

With the user/1.json paths: After restws_init(), within menu_get_item(), $router_items is empty. But that doesn't happen for node paths.

Applying the patch https://www.drupal.org/node/2484829#comment-10284967 makes the module work for Drupal 7.39.

I'm still struggeling with getting this working. I applied path in #67, but the /node/%.json requests are still returning 404s.
Actually like the test results of the patch suggest, in combination with Basic Auth you still get 404s.

It looks like the way of setting the page callback in the patch it not maintained throughout the request.

The menu_item gets set in restws_init(), but when menu_execute_active_handler() gets called => menu_get_item() is called => the value is not set anymore..

I'm not sure, but could be related to this: https://www.drupal.org/node/1613104 (I guess this has been mentioned before)

The value is not set anymore..

Any ideas why?

No (not yet).

When I add the restws_menu_get_item_alter() and the restws_resource_load() from the patch in #39 it works, but I guess this results in the problems already mentioned.

Found it!

The restws_basic_auth module does a drupal_static_reset() after a successful login, this causes the rewritten menu_get_item data (stored using drupal_static()) to be set to 'false'. The git history for this line of code traces back to the first commit of this submodule, so I'm not sure why this line is called anyway.

I can imagine a drupal_static_reset being necessary here, this leaves us with a couple of options:

1. Remove the drupal_static_reset() completely from the restws_basic_auth_init() hook
2. Hack around this call to fetch the 'menu_get_item' data before the reset, and set it again after the reset.
3. Don't use the restws_init() hook to rewrite this

Option 1 i don't know if this leads to any security or other issues.
Option 2 will work but feels hacky.
Option 3 i don't have a clue how to fix this is any other way.

Ok, Option 1 won't work.

I tried Option 2 and got it working in a relatively clean way. I just redetermine the page callback after the drupal_static_reset in restws_basic_auth_init().

See the patch attached.

Will look at this later. Thanks for moving it along.

But, this issue isn't about basic auth, so just from reading this on my phone at the moment, I don't see the how basic auth is relevant.

The patch already fixed node/#.json but user/#.json was not working. Did you try user/#.json?

Well, the previous patch did fix the node/#.json calls, but not when you were using the (submodule) restws_basic_auth, therefore I think this is relevant to this issue.

With my patch the user/#.json calls also works, as long as you have the needed permissions ('View user profiles' and 'Access to the resource user'). And the previous patch also works on user/#.json calls, but you need to give these permissions to the anonymous user.

Mind you that I have only tested the GET requests for these resources.

Alright, thanks for explaining. I had created a separate issue for that actually, but I guess we should combine it all here so that we have a patch that passes tests. Adding the related issue.

Providing an interdiff for the last change.

I agree kurkuma, but my patch passes the tests right?

Agreed. Let's test it and review it.

@lokapujya we need more people review this before we can mark it as reviewed.

I'm ok with giving it some more time for testing. But this functionality is critical to this module. The actual fix for the resource format was coded a month ago. We only just recently fixed the basic authentication. I'll wait a few days, and if no one moves it to needs work, then move it back to RTBC.

The regex seems like it could be cleaned up a bit, whats the reasoning for this '[1-9][0-9]*' ?

Also add some comment above the regex builder to explain the logic in that regex

I guess the reasoning behind that part is that an ID cannot start with a zero (hence the 1-9) but can contain zeros (obviously).

yup cool, so definitely needs some commenting there

Added some commenting and renamed the function.

So for sites that have both enabled this will double call _restws_determine_router_item in init hooks? Is there a reason we can't static cache this function since it should resolve to the same thing twice? Is it that user basic auth is failing test without this because it's firing before the restws does? Could also check for this function existing and making sure it doesn't double fire. Yes, marginal gain but no need to double call this unless I'm missing something

So for sites that have both enabled this will double call _restws_determine_router_item in init hooks?

Yes.

Is there a reason we can't static cache this function since it should resolve to the same thing twice?

It's because there is not much to statically cache imho. The function checks if it's OK to set the current page callback to something, and to do that is uses data from restws_get_resource_info() but not much else. The only thing to be statically cached could be the info whether or not the page callback should be overwritten.

Is it that user basic auth is failing test without this because it's firing before the restws does?

No, it's because it's firing after restws. The restws_basic_auth does a reset after successful login and overwrites the data set in restws_init. Therefore we perform this again.

All in all, in order to statically cache that function, we have to make sure the info that is being checked in the function is equal at both times.

my previous questioning aside (cause I agree w/ #98 was just probing) is that this should be ready to go if it's finally passing the QA bot. I've been running it in prod without the basic_auth portion for several months so just +1ing this

@btopro Several months? But the patch in #95 only came in 15 days ago

sry, overlooked last stamp, rolling 53 which isn't terribly different but is different none the less.

Finally ran into the edge-case expressed that needed 95 to pass. if restws fired before restws_basic_auth it would fail.

Rolled #95 and web calls appear to be working; will need to generate the edge-case where I was triggering this. Lazy fix for that instance was to edit system table and make restws_basic_auth -1 weight.

@btopro so you can confirm you found an issue, even if it's an edge case?

Sorry I was unclear. I was previously using an older patch, your patch fixes the problem more thoroughly / resolves the node/12.json throwing a 404 error when restws_basic_auth is enabled. Patch seems good to go from my perspective

This is not a problem of any specific drupal core version, as still I have got the problem in Drupal-7.31..
patch https://www.drupal.org/node/2484829#comment-10452911 works for me

Thanks the patches do work.

#95 got me out of the woods as well. Thanks all.

RTBC++

#95 fix this issue for me on Drupal v7.43

Unfortunately, we cannot actually commit the fix.

Why not?

No one has access.

@fago @klausi @sepgil Please help

I manually applied the patch in #95 to a vanilla instance of 7.43 with the minimal set of modules installed (arc2_store ctools entity rdfx restws) and enabled. (I have also an 'identical' 7.35 instance for comparison.) We are working on this because we need to produce .rdf output.

I am a relative newbie, but as far as I can tell the patch had no effect. Going to node/1/rdf is just like going to node/1, node/1.rdf elicits a 'page not found'. (Also, node/1?rdf is like going to node/1. I had to try... )

I also tried node/1.json with the same effect but I may not have everything installed or set up for that.

Of course the 7.35 version works fine.

One thing I wasn't sure of - I saw that #95 includes the restws patch file, and the interdiff file - never having done patches I wasn't sure if I was supposed to do that as well. All the changes in that file seemed to be the same as the patch file.

Check all your rest settings from 7.35. Maybe you don't have some of those settings in the 7.43 site.

You should apply the .patch file. The interdiff file is just the changes between that patch and the previous developmental patch.

lokapujya you are correct. :) I was skeptical because there aren't really any relevant configuration settings for RESTws, but I did a side by side compare of enabled modules (flip back and forth between tabs is useful for this), and discovered that two items were different. The first was the Update manager, which should not be relevant, but the second was 'Bulk Exports' in Chaos Tools, which was turned off in the 7.43 instance. When I enabled that module things started to work.

Trying on one of our dev sites, even /node/1234 (without the RDF) is eliciting 404, but that's a separate problem! :) So thanks for the suggestion.

yeah, I don't really like the solution, but it this seems to be the best we can do. Given that I think it makes sense to commit this and roll another release.

I don't think that affects drupal.org as it uses a different api "endpoint" though?

Thanks for reporting, reviewing, and testing! Committed to 7.x.2.x.
A new release is now available with just this commit.

Using the latest stable or dev versions (from Mar 30) and drupal 7.43 if I am to go to something like /node/123.json I am just being redirected to the alias of node/123.

I cannot apply the patch from #95 as it is already in this code base but I still am not getting any json representation of data. Is anyone else still having issues of this VERY basic usage not working? This should be a no config add-on to the site as stated in the instructions. Drop in, turn on, verify permissions....????....profit.

So what gives on the run around of the node/xxx.json end points still just redirecting to the content?

Exactly same situation for me as @majorhallux at post #123 describes.

Same here. #124

confirmed #123 - this patch doesn't work, at least not for my multilingual site with a language path prefix of /en/

I still get a 404 when asking for a node in JSON representation with the latest Drupal core release (7.56) and the 2.7 module release? I also run a multilang site (3 languages, German is the default language, EN and FR the additional languages).

What is recommended to get this functionality back again? It worked for me like a charm until last summer (August 2016) when I did the last export ...

Any advice is much appreciated!

Some more information investigation is needed. The last two reports are from multilang sites.
1. Verify that it does work without multilang, just to make sure you are configuring everything correctly.
2. Verify that it doesn't work with multilang.
3. Open a new issue. Link it to this issue.
4. Try to look at the code change in this issue and see if there is a reason that it wouldn't work with multilang.

I realized that URLs which contain the language shortcut e.g. /en/node/1.json do not work or generate a 404. But if the language shortcut is omitted e.g. /node/1.json the JSON representation of the node is back again (works).