In upgrading to the latest security release of Services 3.x from (2.x), I was confused by several references to the REST Server submodule of Services. Because of these references to a module which I had previously not used, and I mistakenly misunderstood the updated Serivces project page, I incorrectly assumed that the module itself was the Drupal.org module of the same name, and was being developed as it's own standalone project.

This caused a bit of confusion, and I think that others may also be confused by this overlap. I'm not sure the best path forward here, but I think that it might be worthwhile to consider renaming the "included" REST Server module to something unique -- that is, to something other than the existing Drupal.org project, or to figure out a way to test to make sure the enabled module is in fact from "services core", and display a warning or error to users who may have fallen into the same trap.

Also, it may be worthwhile to expand on this caveat information in README.txt.

Thanks!

CommentFileSizeAuthor
#13 1327674-non-core-rest-server-detection-12.patch1.18 KBrypit
#10 1327674-non-core-rest-server-detection-10.patch1.17 KBrypit
#9 1327674-non-core-rest-server-detection-9.patch1.18 KBrypit
#6 1327674-non-core-rest-server-detection-6.patch1.16 KBrypit
#5 1327674-non-core-rest-server-detection-5.patch1.16 KBrypit

Comments

ygerasimov’s picture

ygerasimov commented
Status: Active » Closed (won't fix)

There is no upgrade path from services 2.x to services 3.x (and it is stated on the front page of the project). Also if we are talking about upgrade we should remove old code and then place new code and not put both old code and new code next to each other. Please take a look at http://drupal.org/node/672472

As we don't have upgrade path, I am wondering whether you got your resources working under 3.x after "upgrading" from 2.x?

Marking issue as won't fix.

rypit’s picture

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

I'm aware of module update procedures, but thank you for the relevant link.

After rooting through my site's modules folder and discovering that the updated Rest Server lives inside of the new Services release, I was able to get my environment "working". (There is still a lot I'll need to do in order to get my site back to the state it was in under 2.x, but that is/was expected).

The problem lies in the fact that the 2.x version of REST Server is in fact it's own project (http://drupal.org/project/rest_server), whereas the 3.x version is an included submodule inside of the services module folder.

The problem that this may present to some users is that following normal upgrade procedures: removing the "old" services module and adding the "new" -- or using drush to "drush dl services" -- results in conflict. The new services module replaces the old, but because the difference in module directory structures, the old Rest Server module remains intact (and enabled, in my case -- causing errors).

To improve this experience for users -- and keeping in mind that 3.x is considered a security update (thus many people will be prompted to download and enable it, as 2.x is no longer available AND more is flagged as "insecure"), it would make sense to point out that the new file structure dictates that the Rest Server standalone project also needs to be removed in the "upgrade process" somewhere in the documentation.

So my request then, in the interest of people who intend to switch to the 3.x branch (ie: everybody who intends to use services moving forward), would be to make people aware of this possible (extremely probable) caveat, either on the project page, in the README or both, and ideally to add functionality in the new branch of the module to detect and display warnings to users about conflicts that would arise.

I understand that it is stated that there isn't an upgrade path, but most people update their modules when they see a security warning, and this unintended and undesired behavior is something that at minimum should be documented somewhere. I don't believe that marking this issue (documentation request) as won't fix is a friendly thing to do for the user base of this project.

If you are interested, I would be willing to provide documentation for this, and would also be willing to submit code to services that would detect this probable gotcha and display a friendly warning to users.

It seems that this could be as simple as adding a check in hook_init... that however is at the maintainer's discretion.

marcingy’s picture

marcingy commented
Status: Needs review » Needs work

Moving to needs work as there is not to 'review'. The hook_init idea while good conceptually is going to be a performance issue going forwards because a check is going to have to take place on every page on the site. The impact of this approach could maybe reduced by doing a runtime hook_requirements check and only performing the query if the user is in a certain role.

rypit’s picture

rypit commented

I like this idea -- I'll see what I can whip up tomorrow :)

rypit’s picture

rypit commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
StatusFileSize
new 1327674-non-core-rest-server-detection-5.patch1.16 KB

This was incorrectly stated as an issue for 7.x-3.x, but I actually experienced the problem on 6.x-3.x. This problem will not occur on 7.x as long as there is no 7.x version of the Rest Server Project.

Attached is a patch for the 6.x-3.x version. It compares the installed Rest Server module's location to the known location of the core Rest Server module and generates an error in hook_requirements there is a difference. Please let me know if you spot any problems.

rypit’s picture

rypit commented
Status: Needs work » Needs review
StatusFileSize
new 1327674-non-core-rest-server-detection-6.patch1.16 KB

Reuploading patch and changing status to needs review so that the bot can see it.

marcingy’s picture

marcingy commented

Thanks for the patch I have given it a quick initial review and there are a few code style issues but the patch generally looks good.

//if rest_server is enabled, make sure it's from core

Should be

// If rest_server is enabled, make sure it's from core.

Maybe also we should use the phrase services core rather than core as that makes me think of drupal.

  if( module_exists('rest_server') ) {

should be

  if (module_exists('rest_server')) {

  $core_path = drupal_get_path('module', 'services') . '/servers/rest_server';

should be (and yeah for stupid standard that was removed in d7!!

  $core_path = drupal_get_path('module', 'services'). '/servers/rest_server';

  if( $rest_path !== $core_path ) {

should be

  if ($rest_path !== $core_path) {

  'description' => $t('The enabled version of Rest Server is not from Services core. Uninstall then remove the old Rest Server module from') . ' ' . $rest_path . ' ' . t('to utilize Services core Rest Server'),

should be

  'description' => $t('The enabled version of Rest Server is not from Services core. Uninstall then remove the old Rest Server module from'). ' '. $rest_path. ' '. t('to utilize Services core Rest Server'),
marcingy’s picture

marcingy commented
Status: Needs review » Needs work
rypit’s picture

rypit commented
Status: Needs work » Needs review
StatusFileSize
new 1327674-non-core-rest-server-detection-9.patch1.18 KB

Patch with stylistic changes and improved comment is attached :)

rypit’s picture

rypit commented
StatusFileSize
new 1327674-non-core-rest-server-detection-10.patch1.17 KB

patch #9 is bad, please use this one instead.

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Needs work

@marcingy I believe we still should have spaces on both sides around concatenation operator "."
See http://drupal.org/coding-standards#concat

rypit’s picture

rypit commented
Status: Needs work » Needs review
StatusFileSize
new 1327674-non-core-rest-server-detection-12.patch1.18 KB

Attached patch follows @ygerasimov's link and adds spaces before concat operators...

marcingy’s picture

marcingy commented

Not in d6 the standard was changed in d7, and the old style is the standard used by core. The patch is #10 is correct which will be the one I commit.

marcingy’s picture

marcingy commented
Status: Needs review » Fixed
rypit’s picture

rypit commented

Either way, glad to help :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.