Posted by cotto on November 23, 2011 at 12:52am
6 followers
| Project: | Services |
| Version: | 6.x-3.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| services-server-settings-7.x.patch | 7.15 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in services-server-settings-7.x.patch. | View details | Re-test |
| services-server-settings-6.x.patch | 6.46 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in services-server-settings-6.x.patch. | View details | Re-test |
Comments
#1
These look good, but Id like marcingy to review first.
#2
Assigning so I don't forget
#3
I can't apply patch for 7.x branch:
fatal: corrupt patch at line 216Could 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!
#4
Here's a rerolled version for 7.x-3.x .
#5
I am happy with patch of 7.x branch except missing '}' in .install file. Attached rerolled patch. All tests pass.
#6
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.
#7
The attached version should be upgrade-safe, in that upgrade 7400 can be run multiple times without causing any damage.
#8
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.
#9
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.
#10
Much happier with the new update approach. Few issues though
We have a typo I assume remove?
<?php// Note, this will not removw previous authentication settings, it will
?>
And
<?phpif (in_array( key($settings), $server_names)) {
?>
should be
<?phpif (in_array(key($settings), $server_names)) {
?>
Otherwise I'm happy with this patch it is a nice clean up.
#11
#12
An updated patch is attached. Thanks for the suggestions. I'm glad not to have my typos preserved for posterity.
#13
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
#14
If tests pass then I'm happy to rtbc this, I might get a chance to run it locally tomorrow.
#15
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.
#16
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.
#17
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.
#18
#19
There is a typo
+ * Update 3605 reduces nesting in the way server settings are storedAlso 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 :)
#20
fixed in the attached patch (also rebased against 6.x-3.x)
#21
#22
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.
#23