Lets us drupal_static in function services_server_info_object() instead of d6 approach static. Also add functionality of $reset key.

CommentFileSizeAuthor
#17 fast-static-2.patch614 bytesmarcingy
#11 fast-static.patch649 bytesmarcingy
#4 0001-Add-rest-to-services_server_info_object.patch713 bytesygerasimov
services_server_info_object-drupal_static.patch926 bytesygerasimov

Comments

marcingy’s picture

marcingy commented
Status: Needs review » Needs work

I wonder if we should infact use the fast static pattern here, given the number of times this function is called during the services processes.

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review

I think we should follow d7 pattern on this. Also $reset functionality is enabled with this patch.

marcingy’s picture

marcingy commented
Status: Needs review » Needs work

Drupal fast static is a drupal pattern see second example on this page http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal...

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review
StatusFileSize
new 0001-Add-rest-to-services_server_info_object.patch713 bytes

Alright. Lets keep it $static as we are sure nobody will need to reset this cache from other places. Attached patch only adds usage of $reset key.

marcingy’s picture

marcingy commented
Status: Needs review » Needs work

We want to do this

  static $drupal_static_fast;
  if (!isset($drupal_static_fast)) {
    $drupal_static_fast['perm'] = &drupal_static(__FUNCTION__);
  }

So we still have a drupal based static that can be reset from every plus the additional benefit of localised static.

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review

@marcingy truly to say #5 looks more complicated that it is now. I wouldn't go for this way. Lets keep it simple whether using drupal_static() or simple static.

Lets just commit #4 adding $rest key and thats it.

marcingy’s picture

marcingy commented
Status: Needs review » Needs work

Yes it is more complex but it also then allows the usage of drupal_static_reset and it is only more complex from an implemtation point of view and we only have to do it once. So lets do this properly.

kylebrowning’s picture

kylebrowning commented

where we at with this?

kylebrowning’s picture

kylebrowning commented
Assigned: Unassigned » marcingy
ygerasimov’s picture

ygerasimov commented

Kyle, I think we should go forward on this with original patch. Simple changing to using drupal_static().

marcingy’s picture

marcingy commented
StatusFileSize
new fast-static.patch649 bytes

Fast static version

marcingy’s picture

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

ygerasimov commented
Status: Needs review » Needs work

Never seen such pattern.

+++ b/services.runtime.incundefined
@@ -307,11 +307,12 @@ function services_get_server_info($key, $default = NULL) {
+    $drupal_static_fast['info'] = &drupal_static(__FUNCTION__);
+    $drupal_static_fast['info'] = new stdClass();

Why do we need &drupal_static here as we overrite it with empty object on the next line?

marcingy’s picture

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

The answer is we need a fast static the code may need work but original patch should not be committed is my point....and I rolled this patch despite being up since 6 am my time to prevent this happening due to the comment in #10 ie the original patch is wrong and have said this lots of times. But what ever commit what ever you want.....

marcingy’s picture

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

The problem is that you can't provide a new class as a default value to drupal static, the former is creating a reference to the varaiable the latter is getting round the initialisation issue described above as you have to initialise to an array or string. If you can do the operations in one sure go for it. And if you grep drupal_static_fast you will find its usage in multiple places in core. The idea is that provides the benefits of a localised static along with the ability to perform a centralised reset.

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Needs work

so whats going on with this?

marcingy’s picture

marcingy commented
Status: Needs work » Needs review
StatusFileSize
new fast-static-2.patch614 bytes

So it turns out this funky approach does actually work, don't you love php at times!

kylebrowning’s picture

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

marcingy commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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