Beware of static and unset together
Hi, So says the PHP manual about unset "If a static variable is unset() inside of a function, unset() destroys the variable only in the context of the rest of a function. Following calls will restore the previous value of a variable." This will lead to really weirdo stuff if you have: function foo($refresh = FALSE) { static $storage; if ($refresh) { echo "refreshing\n"; unset($storage); } if (!isset($storage)) { $storage = microtime(); } return $storage; } echo foo() ."\n"; echo foo(TRUE) ."\n"; echo foo() ."\n"; Observe how the third and the first results are the same as if the refresh did not happen. As Drupal often uses such constructs. Use $storage = NULL instead of unset, for example -- or some other appropriate initial value like '' or array(). list_themes() in core for example has been biten by this, thanks goes to dww for reporting it. The patch is at http://drupal.org/node/134161 btw. I think it's better known that if you have global $x; unset $x; then $x will be unset only in local scope, as global $x is same as saying $x = &GLOBALS['x']; The above issue is actually the same because -- according to my knowledge -- static is a global which has a special flag 'you are only visible to function foo', so basically you have $storage = &$GLOBALS['_static']['foo']['storage']; and then you destroy this reference (of course GLOBALS['_static'] does not exist, I just used it for illustration). Regards, NK
Thanks for pointing this out. Semantically, using unset() on a static is an oxymoron. More strongly typed languages would probably puke at compile time on such a thing.
On Fri, 2007-04-06 at 00:45 -0700, Karoly Negyesi wrote:
Hi,
So says the PHP manual about unset
"If a static variable is unset() inside of a function, unset() destroys the variable only in the context of the rest of a function. Following calls will restore the previous value of a variable."
This will lead to really weirdo stuff if you have:
function foo($refresh = FALSE) { static $storage; if ($refresh) { echo "refreshing\n"; unset($storage); }
if (!isset($storage)) { $storage = microtime(); } return $storage; } echo foo() ."\n"; echo foo(TRUE) ."\n"; echo foo() ."\n";
Observe how the third and the first results are the same as if the refresh did not happen.
Sounds like a good excuse for properly initializing your static's and reinitializing them when the need to be reset.
Custom theme settings are something that are currently possible, but don't work terribly well in PHPTemplate. Although it's not documented, you can add a phptemplate_settings() function to your theme's template.php and add custom settings. BUT, you can't edit settings of non-active themes because you would get function re-declaration errors if you were to try including both the active and non-active theme's template.php file. So a separate file is really needed to hold each theme's settings. I believe the best solution to this issue is to: 1. Change system_theme_settings() to allow both engine-specific and theme-specific settings for any theme. Currently, if the theme is based on an engine, system_theme_settings() will only add engine-specific settings. 2. Add phptemplate_settings() to the PHPTemplate engine and, using the $key passed by system_theme_settings(), look in the $key theme's directory and include and return the contents of a settings.php file. I've made a patch for Drupal 6. If you want to review, contribute, (or commit :-D ), take a look at the issue, patch and example settings.php file here: http://drupal.org/node/57676 - John (JohnAlbin) BTW, if you want a solution to this issue for Drupal 5, see my Theme Settings API module at http://drupal.org/project/themesettings
Funny how you created a "themesettings" module that duplicates my own "themesettings" module.. http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/fgm/themesettings... Is it the same mechanism/code ? ----- Original Message ----- From: "John" <drupal.user@albin.net> To: <development@drupal.org> Sent: Friday, May 11, 2007 8:57 AM Subject: [development] Custom Theme Settings
Custom theme settings are something that are currently possible, but don't work terribly well in PHPTemplate.
Although it's not documented, you can add a phptemplate_settings() function to your theme's template.php and add custom settings. BUT, you can't edit settings of non-active themes because you would get function re-declaration errors if you were to try including both the active and non-active theme's template.php file. So a separate file is really needed to hold each theme's settings.
I believe the best solution to this issue is to:
1. Change system_theme_settings() to allow both engine-specific and theme-specific settings for any theme. Currently, if the theme is based on an engine, system_theme_settings() will only add engine-specific settings. 2. Add phptemplate_settings() to the PHPTemplate engine and, using the $key passed by system_theme_settings(), look in the $key theme's directory and include and return the contents of a settings.php file.
I've made a patch for Drupal 6. If you want to review, contribute, (or commit :-D ), take a look at the issue, patch and example settings.php file here: http://drupal.org/node/57676
- John (JohnAlbin)
BTW, if you want a solution to this issue for Drupal 5, see my Theme Settings API module at http://drupal.org/project/themesettings
On May 11, 2007, at 12:27 AM, FGM wrote:
Funny how you created a "themesettings" module that duplicates my own "themesettings" module..
Is it the same mechanism/code ?
Federick, head on over to the Theme Settings project and take a look at the themesettingsapi.module code yourself: http://drupal.org/ project/themesettings Also, look at the project home page under "About the Project" and you'll see I referenced issue #54990 where you announced your sandbox module. It's not the same code, but even if it was... aren't we all writing Open Source software? :-) - John
John wrote:
It's not the same code, but even if it was... aren't we all writing Open Source software? :-)
Yes, but Drupal encourages collaboration to help reduce the enormity of user confusion of having too many versions of the same task. We've been losing this over time as more and more duplicate modules are creeping in, but we do *try* to convince duplicate module owners to collaborate and reduce the number of paths.
Right. What Earl said. Except that if Fergus expects John to have looked into Fergus' sandbox before beginning development on this, well, that's not realistic. If I were in John's shoes I would have ended up writing a new module, too. -Robert Earl Miles wrote:
John wrote:
It's not the same code, but even if it was... aren't we all writing Open Source software? :-)
Yes, but Drupal encourages collaboration to help reduce the enormity of user confusion of having too many versions of the same task. We've been losing this over time as more and more duplicate modules are creeping in, but we do *try* to convince duplicate module owners to collaborate and reduce the number of paths.
Robert Douglass wrote:
Right. What Earl said. Except that if Fergus expects John to have looked into Fergus' sandbox before beginning development on this, well, that's not realistic. If I were in John's shoes I would have ended up writing a new module, too.
Good point. Sandboxes are kind of invisible.
We've kind of gone down a rabbit hole for a topic that was just a PS on my original post. I would like to (again) encourage anyone interested in seeing Custom Theme Settings in Drupal 6 to please review and test the patch here: http://drupal.org/node/57676 Only 3 weeks to code freeze! - John
Quoting Robert Douglass <rob@robshouse.net>:
Right. What Earl said. Except that if Fergus expects John to have looked into Fergus' sandbox before beginning development on this, well, that's not realistic. If I were in John's shoes I would have ended up writing a new module, too.
I remember some discussion when I first started this list about the removal of sandboxes. Could this have been the reason? If the code is good enough to be used then make it a module and don't hide it; IIRC, was stated by someone.
-Robert
Earl Miles wrote:
John wrote:
It's not the same code, but even if it was... aren't we all writing Open Source software? :-)
Yes, but Drupal encourages collaboration to help reduce the enormity of user confusion of having too many versions of the same task. We've been losing this over time as more and more duplicate modules are creeping in, but we do *try* to convince duplicate module owners to collaborate and reduce the number of paths.
How can this be controlled? What methods could be put into place to help reduce the duplication? Self policing doesn't work well for these situations. We need some stronger methods to control the desire to create YA module of the same function. Earnie
On May 12, 2007, at 8:06 AM, Earnie Boyd wrote:
How can this be controlled? What methods could be put into place to help reduce the duplication? Self policing doesn't work well for these situations. We need some stronger methods to control the desire to create YA module of the same function.
Actually, Frederic and I exchanged about 4 emails on April 8 regarding the 2 themesettings modules. But it seems he has forgotten. So self policing actually did work in this situation; we were both aware of each other's code and I invited him to collaborate. I agree with Earnie that 2 YA-solution _projects_ are indeed a problem, but 1 project and 1 sandbox module... not so much. Collaboration & the sandbox: What are the uses of the CVS sandbox? If the sandbox is supposed to be a place where people can collaborate before a project is created, perhaps a CVS repository associated with the groups.d.o website would be more useful. Code duplication: Perhaps it needs to be a requirement that module developer's announce their project on this list BEFORE any code becomes public on the d.o website. I'm not sure how we could enforce that requirement, but perhaps just documenting it in the appropriate places in the handbook would be enough. Any other suggestions? - John
I agree. I think an "announcement" of some sort before going live would benefit to help reduce code duplication. I know that I am just as much adding to the problem as anyone else (e.g. with my pontomail, I should have used MimeMail on the send part of the module instead of writing my own sendmail function...currently refactoring/writing). I would like to see drupal go the way of the unix methodology, meaning many small tools, which when used in collaboration, can do powerful things. This doesn't mean an explosion of small simplistic modules, what I mean by this is there are some module functionalities that should be "generalized" into an api, and used by other modules so that we don't duplicate the same functionality. Token module is a good example. I think there should be more of these "API" modules, so that we're "extending" functionality, not re-writing functionality. Just my 2 cents. Now, how to go about controlling this and implementing this is another discussion. -----Original Message----- From: development-bounces@drupal.org [mailto:development-bounces@drupal.org] On Behalf Of John Sent: Saturday, May 12, 2007 9:54 PM To: development@drupal.org Subject: [development] Code duplication, collaboration,and sandboxes. Oh my! (was: Custom Theme Settings) On May 12, 2007, at 8:06 AM, Earnie Boyd wrote:
How can this be controlled? What methods could be put into place to help reduce the duplication? Self policing doesn't work well for these situations. We need some stronger methods to control the desire to create YA module of the same function.
Actually, Frederic and I exchanged about 4 emails on April 8 regarding the 2 themesettings modules. But it seems he has forgotten. So self policing actually did work in this situation; we were both aware of each other's code and I invited him to collaborate. I agree with Earnie that 2 YA-solution _projects_ are indeed a problem, but 1 project and 1 sandbox module... not so much. Collaboration & the sandbox: What are the uses of the CVS sandbox? If the sandbox is supposed to be a place where people can collaborate before a project is created, perhaps a CVS repository associated with the groups.d.o website would be more useful. Code duplication: Perhaps it needs to be a requirement that module developer's announce their project on this list BEFORE any code becomes public on the d.o website. I'm not sure how we could enforce that requirement, but perhaps just documenting it in the appropriate places in the handbook would be enough. Any other suggestions? - John
Earnest Berry III wrote:
I agree. I think an "announcement" of some sort before going live would benefit to help reduce code duplication. I know that I am just as much adding to the problem as anyone else (e.g. with my pontomail, I should have used MimeMail on the send part of the module instead of writing my own sendmail function...currently refactoring/writing).
Also some auditing of module categorization would help. I just emailed a module author today as their module isn't placed in the most logical category. Because of this I missed their module and ended up coding a custom, redundant module. Mike
Hi all in this thread, Just to clarify some points, which I think may be needed to spare some anguish, seeing how the thread appears unduly long: - When I saw this announcement, I was indeed surprised to see the duplication, and checked the three issues mentioned by John but, alas, I overlooked part of 54990 and thought he had not taken into account my remark on that issue, that pointed to the module in my sandbox. My fault. - I certainly do not attempt to deny anyone the idea of creating such a project. Indeed on the page where I announce the themesettings module I had in my sandbox, I explain rather clearly that it is just a "proof-of-concept" module, until something appears in core (which I hoped for 5.0, seeing how garland had its own settings mechanism, although done differently). So, since this - As John said, "we are all writing Open Source software", so I answered to the announcement to point to potential duplication, as Earl Miles initially remarked, not for a rights issue, as some others seem to have thought. However, John apparently was aware of this existing module, and indeed uses the same mechanism, in a much more developed code, in his function themesettingsapi_form_alter, so apparently there is no duplication, just a (very significant) functionality and packaging extension. - In the end, I think I was a bit miffed to discover a module by the same name as mine in the dev list, after the fact without any personal mail from the author especially since we had indeed exchanged some emails five weeks ago. So I think that to avoid similar problems for future occurences like this, John's latest suggestion in this thread makes sense: "Code duplication: Perhaps it needs to be a requirement that module developer's announce their project on this list BEFORE any code becomes public on the d.o website. [...] perhaps just documenting it in the appropriate places in the handbook would be enough." As far as I'm concerned, that's all there is to it, and I'm now pointing users of this sandbox module to his project (I know some exist, because I had received mails about it), because this being a project, it is supposed to be maintained and evolve, unlike a sandbox, which in this case fulfilled what IMHO seems to be its role: show ideas until they become ready for prime time. I'd actually have done it earlier if the module had been preannounced. Frederic ----- Original Message ----- From: "John" <drupal.user@albin.net> To: <development@drupal.org> Sent: Friday, May 11, 2007 10:16 AM Subject: Re: [development] Custom Theme Settings
On May 11, 2007, at 12:27 AM, FGM wrote:
Funny how you created a "themesettings" module that duplicates my own "themesettings" module..
Is it the same mechanism/code ?
Federick, head on over to the Theme Settings project and take a look at the themesettingsapi.module code yourself: http://drupal.org/ project/themesettings
Also, look at the project home page under "About the Project" and you'll see I referenced issue #54990 where you announced your sandbox module. It's not the same code, but even if it was... aren't we all writing Open Source software? :-)
- John
Guys (this is to all of you), you are all doing good work. A little less guilt-tripped bowing down to some god of open source will go a long way here. Don't let anyone guilt-trip you into stuff. I am relatively new to the Drupal community (only a year and a half) but I think the Drupal way is marked by three words: cooperation, selflessness and above all intelligence. Cooperation: If you see someone carrying a load, help her. Selflessness: Take due credit for stuff, but don't do stuff to get credit. Intelligence: The beauty of Drupal is, speaking broadly, separation of model/view/controller and software reuse. On that last point, what you can see is people coming into Drupal thinking that "module" is a block of functionality appearing somewhere on a page. Here, in Drupal-land, a "module" is a group of files mainly concerned with controller logic and the resources necessary for the implementation of this logic, totally separated from general theming, and relying on a database abstraction layer for its persistence. So people generally try to use the accepted standards in accessing the database, and try to use the theming function conventions so that any theming decisions can be overridden. That keeps things clean. And, people try to reuse modules, and that keeps things clean. If, in your Sandbox, you want to play around with some functionality, no-one should stop you, just go ahead and have a ball. Of course, before starting, it is simply good, intelligent software practice to attempt to reuse an existing module rather than inventing the wheel. This is not only to keep things clean. This is to keep things strong by reusing proven code rather than slapping non-proven code into our machines. Then, when it comes time to say, hey this deserves to be a module, common sense would say look around and see what module integration is possible (two examples here are jstools and swftools, among many); talk to people who are doing similar stuff; if you think it is original and useful, publish your module. Then, let people vote with their feet: if two or three similar modules are out there, people will vote with their feet, which is much more effective than having some incipient bureaucracy deem what is fit. Then, usually when a new release comes out the unwanted and unloved modules usually don't get updated, and nature has its course. Case in point: Salesforce module. 1. My client needs salesforce integration urgently for a current project. 2. I prefer to see that done in the form of re-use of a module rather than hacking stuff together individually, so that my client can benefit from community code while the community benefits from my client's project. My client understands this and applauds it. 3. I find a perfectly neat salesforce module in a fellow drupaler sandbox; I see his wishlist saying "gosh darn it, wish I could find someone to take this pesky salesforce module off my hands". I pm him, saying "I'm your man". 4. With Steve McKenzie's permission, I take his sandbox module and import it as a bonafide module. 5. I am now testing it, have corrected some things and will publish shortly 4.7.x and a 5.x releases. 6. I will then extend the module with the additional functionality my client needs (views support for online queries, to name one). Exciting! 7. Then, I get mail and messages saying: hey, what about unifying this with access and integration modules with other CRM's, including CivicCRM, etc. and I answer, cool, let me get this on the road, then let's discuss how that can be done. The more discussion we have on that the better. 8. It's the year 2008. My client decides to switch to CivicCRM or its on-demand (proprietary) version instead of the bad proprietary Salesforce. It is a totally painless operation since all that needs to be done is switch a configuration setting and do some import/export (a feature of the module itself hahaha). So, any Salesforce diehards can stay with the bad proprietary Salesforce, whilst do-gooders can go with the good proprietary CivicCRM on-demand version. Sweet! Drupal does it again! saludos, Victor Kane http://awebfactory.com.ar On 5/13/07, FGM <fgm@osinet.fr> wrote:
Hi all in this thread,
Just to clarify some points, which I think may be needed to spare some anguish, seeing how the thread appears unduly long: - When I saw this announcement, I was indeed surprised to see the duplication, and checked the three issues mentioned by John but, alas, I overlooked part of 54990 and thought he had not taken into account my remark on that issue, that pointed to the module in my sandbox. My fault. - I certainly do not attempt to deny anyone the idea of creating such a project. Indeed on the page where I announce the themesettings module I had in my sandbox, I explain rather clearly that it is just a "proof-of-concept" module, until something appears in core (which I hoped for 5.0, seeing how garland had its own settings mechanism, although done differently). So, since this - As John said, "we are all writing Open Source software", so I answered to the announcement to point to potential duplication, as Earl Miles initially remarked, not for a rights issue, as some others seem to have thought. However, John apparently was aware of this existing module, and indeed uses the same mechanism, in a much more developed code, in his function themesettingsapi_form_alter, so apparently there is no duplication, just a (very significant) functionality and packaging extension. - In the end, I think I was a bit miffed to discover a module by the same name as mine in the dev list, after the fact without any personal mail from the author especially since we had indeed exchanged some emails five weeks ago. So I think that to avoid similar problems for future occurences like this, John's latest suggestion in this thread makes sense:
"Code duplication: Perhaps it needs to be a requirement that module developer's announce their project on this list BEFORE any code becomes public on the d.o website. [...] perhaps just documenting it in the appropriate places in the handbook would be enough."
As far as I'm concerned, that's all there is to it, and I'm now pointing users of this sandbox module to his project (I know some exist, because I had received mails about it), because this being a project, it is supposed to be maintained and evolve, unlike a sandbox, which in this case fulfilled what IMHO seems to be its role: show ideas until they become ready for prime time. I'd actually have done it earlier if the module had been preannounced.
Frederic
----- Original Message ----- From: "John" <drupal.user@albin.net> To: <development@drupal.org> Sent: Friday, May 11, 2007 10:16 AM Subject: Re: [development] Custom Theme Settings
On May 11, 2007, at 12:27 AM, FGM wrote:
Funny how you created a "themesettings" module that duplicates my own "themesettings" module..
Is it the same mechanism/code ?
Federick, head on over to the Theme Settings project and take a look at the themesettingsapi.module code yourself: http://drupal.org/ project/themesettings
Also, look at the project home page under "About the Project" and you'll see I referenced issue #54990 where you announced your sandbox module. It's not the same code, but even if it was... aren't we all writing Open Source software? :-)
- John
participants (12)
-
Chris Johnson -
Darrel O'Pry -
Earl Miles -
Earnest Berry III -
Earnie Boyd -
FGM -
John -
John Wilkins
-
Karoly Negyesi -
Mike Cantelon -
Robert Douglass -
Victor Kane