Currently, there's an unnecessary level of nesting in the server_settings member of the $endpoint object. It smells like a historical leftover, makes the code harder to read than necessary and causes a PHP notice every time an endpoint is hit until an admin hits the save button on the server edit form. The attached patches against 7.x-3.x and 6.x-3.x fix this and include an update to clean up existing endpoints.

CommentFileSizeAuthor
#20 services-server-settings-6.x-v3.patch6.48 KBcotto
#17 services-server-settings-6.x-v2.patch6.65 KBcotto
#12 services-server-settings-7.x-v5.patch8.49 KBcotto
#9 services-server-settings-7.x-v4.patch8.49 KBcotto
#7 services-server-settings-7.x-v3.patch8.43 KBcotto
#5 services-server-settings-7.x-v2-rerolled.patch8.16 KBygerasimov
#4 services-server-settings-7.x-v2.patch8.02 KBcotto
services-server-settings-6.x.patch6.46 KBcotto
services-server-settings-7.x.patch7.15 KBcotto

Comments

kylebrowning’s picture

kylebrowning commented

These look good, but Id like marcingy to review first.

marcingy’s picture

marcingy commented
Assigned: Unassigned » marcingy

Assigning so I don't forget

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Needs work

I can't apply patch for 7.x branch:

fatal: corrupt patch at line 216

Could you please reconfirm whether it is my local problem or patch needs reroll? I really would like to test it.

I believe this patch will also solve exceptions in our tests. Great job @cotto!

cotto’s picture

cotto commented
StatusFileSize
new services-server-settings-7.x-v2.patch8.02 KB

Here's a rerolled version for 7.x-3.x .

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review
StatusFileSize
new services-server-settings-7.x-v2-rerolled.patch8.16 KB

I am happy with patch of 7.x branch except missing '}' in .install file. Attached rerolled patch. All tests pass.

marcingy’s picture

marcingy commented

I have given this a review and am happy. I do have one question however what happens if someone upgrades for services 6.x.3 to 7.x.3 will this update work as expected. If the answer to this is yes I'm happy to rtbc.

cotto’s picture

cotto commented
StatusFileSize
new services-server-settings-7.x-v3.patch8.43 KB

The attached version should be upgrade-safe, in that upgrade 7400 can be run multiple times without causing any damage.

marcingy’s picture

marcingy commented
Status: Needs review » Needs work

can we not do $servers = services_get_servers(); and then do a test to see if $endpoint->server_settings[array_value] is in the list of returned servers rather than depending on a value not being set.

cotto’s picture

cotto commented
StatusFileSize
new services-server-settings-7.x-v4.patch8.49 KB

Here's a version with a more paranoid update_7400, as requested. As with the previous patch, it should be safe to run this more than once without causing any unwanted side-effects. The update seems to work correctly on my test d7 site for endpoints created both before and after applying the patch, but that doesn't make further review a bad idea.

marcingy’s picture

marcingy commented

Much happier with the new update approach. Few issues though

We have a typo I assume remove?

// Note, this will not removw previous authentication settings, it will

And

if (in_array( key($settings), $server_names)) {

should be

if (in_array(key($settings), $server_names)) {

Otherwise I'm happy with this patch it is a nice clean up.

marcingy’s picture

marcingy commented
Assigned: marcingy » Unassigned
cotto’s picture

cotto commented
StatusFileSize
new services-server-settings-7.x-v5.patch8.49 KB

An updated patch is attached. Thanks for the suggestions. I'm glad not to have my typos preserved for posterity.

kylebrowning’s picture

kylebrowning commented

Does this still needs work? It appears that theres some errors on 7.x-3.x and they pretty much are fixed with this patch. :P

marcingy’s picture

marcingy commented
Status: Needs work » Needs review

If tests pass then I'm happy to rtbc this, I might get a chance to run it locally tomorrow.

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Reviewed & tested by the community

Tests pass (1789 passes, 0 fails, 1 exception, and 404 debug messages). One exception is in services.module file and not related to this issue.

marcingy’s picture

marcingy commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

This has been committed for d7 can we have a reroll for d6 and I'll get it committed as soon as possible.

This is a really nice clean up thanks to everyone involved.

cotto’s picture

cotto commented
StatusFileSize
new services-server-settings-6.x-v2.patch6.65 KB

Here's a 6.x-3.x version of the patch. It doesn't cause any new test failures and correctly updated a pre-patch endpoint that I created for testing.

cotto’s picture

cotto commented
Status: Patch (to be ported) » Needs review
marcingy’s picture

marcingy commented
Status: Needs review » Needs work

There is a typo

+ * Update 3605 reduces nesting in the way server settings are stored

Also do we need the conditional check in the d6 update as the update will not have run a previous version of drupal.

Otherwise looks good :)

cotto’s picture

cotto commented
StatusFileSize
new services-server-settings-6.x-v3.patch6.48 KB

fixed in the attached patch (also rebased against 6.x-3.x)

cotto’s picture

cotto commented
Status: Needs work » Needs review
kylebrowning’s picture

kylebrowning commented
Status: Needs review » Reviewed & tested by the community

this patch might break against core, sorry, i think i first the first part without a ticket but, if this still applies its good to go.

kylebrowning’s picture

kylebrowning commented
Status: Reviewed & tested by the community » Needs review
kylebrowning’s picture

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

never got a patch for 6.x