The version of SPYC that's bundled with Services 3.x is inconsistent between the 6.x (0.3) and 7.x (0.4.5) branches and is behind the latest 0.5.0 release. The attached patches update the REST server in 6.x and 7.x.

This also fixes a warning in 3.x-6.x that can be triggered by hitting endpoint/user/login.yaml with a GET, causing SPYC to dump an empty variable. 0.4.5 and 0.5.0 handle this fine but 0.3.0 produces a warning, hence the upgrade. There aren't any major interface changes between 0.4.5 and 0.5, so the 7.x patch just copies the upstream spyc.php into servers/rest_server/lib/spyc.php.

CommentFileSizeAuthor
#12 spyc.php_.txt32.09 KBcotto
#7 services-spyc-object-array-fix.patch571 bytescotto
rest_server_spyc_upgrade-6.x.patch35.79 KBcotto
rest_server_spyc_upgrade-7.x.patch6.12 KBcotto
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Category: bug » task
marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Active » Postponed

We are going to remove these libraries #1325572: Integrate services with the libraries module to improve handling of third party libraries.. They should never have been in repo in the first place.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Postponed » Closed (won't fix)

Libraries no longer ship with services

kylebrowning’s picture

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

Yaml is currently broken, this needs to happen.

marcingy’s picture

marcingy CreditAttribution: marcingy commented

We need to update d6 to work with the latest version of spyc rather than committing the entire patch.

marcingy’s picture

marcingy CreditAttribution: marcingy commented

This this should be in head - http://drupal.org/node/1334784

cotto’s picture

cotto CreditAttribution: cotto commented
FileSize
services-spyc-object-array-fix.patch571 bytes

marcingy, that doesn't seem to be the problem. It looks like the root problem spyc doesn't know how to serialize an array of objects. The following code will also fail.

    $spyc = new Spyc;
    $array = array( );
    $array[0] = new StdClass;
    $array[0]->foo = 'buz';
    return $spyc->dump($array, 0, 0);

The fix for spyc is pretty simple (attached). I've also submitted an upstream issue with the same patch. It's likely to be a while before someone upstream gets around to fixing the issue, so we should probably figure out what to do in the meantime.

marcingy’s picture

marcingy CreditAttribution: marcingy commented

I am not sure how to handle this let me think some more! I'm going to show some ignorance around this library can we make sure we supply an array or are there cases where we need to provide an object. I ask as all data being passed to services should be array anyway.

cotto’s picture

cotto CreditAttribution: cotto commented

The issue isn't the data that's being passed to services, but the data that's being returned.
Converting anything that'll get serialized through spyc into an array is possible, though clunky. I don't see any reason why objects strictly need to be part of the output, but it also seems weak reformat services' data because of a simple upstream bug in spyc.

The best way to deal with this may be to include a patched spyc with Services. If a similar patch gets integrated into upstream spyc, services can go back to requiring the download as it does now.

apotek’s picture

apotek CreditAttribution: apotek commented

The best way to deal with this may be to include a patched spyc with Services. If a similar patch gets integrated into upstream spyc, services can go back to requiring the download as it does now.

I think if someone can post a patched version of Spyc in this thread, that is all that needs to be done. Services should not be including the spyc library in its download. Maybe a posted patch, and a link from the main project description page to that patched version of spyc would tide users over until spyc fixes itself upstream.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Version: 6.x-3.x-dev » 7.x-3.x-dev

The approach described by @apotek sounds like the best option. Moving to d7 as we need fix there as well I'm guessing.

cotto’s picture

cotto CreditAttribution: cotto commented
FileSize
spyc.php_.txt32.09 KB

Here's the patched version of spyc, renamed due to d.o's filename extension limits.

apotek’s picture

apotek CreditAttribution: apotek commented

Thanks! This looks good to me and I've committed this into production.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Active » Reviewed & tested by the community

Marking as rtbc note do not commit this patch ;) Just noting it has sign off and works. I have added a link to the on the main page to this issue.

rypit’s picture

rypit CreditAttribution: rypit commented

Does it make sense to use the Libraries API for all external libraries? library_get_path seems like a nice way to organize things moving forward. It seems like it's a very organized way to handle libraries for multisites especially. Should this be a feature request?

marcingy’s picture

marcingy CreditAttribution: marcingy commented

#15 is total different issue #1325572: Integrate services with the libraries module to improve handling of third party libraries. and nothing to do with the problem here and already has an item in the services queue and patch that needs work. Whether libraries is used or not there is still an upstream issue sypc and this library should not exist any on d.o in a committed state.

kylebrowning’s picture

kylebrowning CreditAttribution: kylebrowning commented
Status: Reviewed & tested by the community » Closed (won't fix)

Im marking this as won't fix, id like #15 to push forward asap.

pcoughlin’s picture

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

I am still not able to get the Status Report warning to go away. Have put spyc.php in every folder I can find a reference to and the warning is still present.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
wal-78’s picture

wal-78 CreditAttribution: wal-78 commented

rest_server_spyc_upgrade-7.x.patch queued for re-testing.

marcingy’s picture

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

Please do not re-open old issues this was solved in a different way using libraries api.

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn commented
Issue summary: View changes
Related issues: +#1349588: Update documentation and make file relating to Spyc requirement and Libraries integration + make libraries optional
Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn commented
Related issues: +#1325572: Integrate services with the libraries module to improve handling of third party libraries.