- Development - lists.drupal.org

[drupal-devel] [feature] 2 additional hooks to form_textarea to support module writers who are extending it.
by Bèr Kessels 06 Mar '05

06 Mar '05

Issue status update for http://drupal.org/node/18417 Project: Drupal Version: 4.5.2 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: jbond Updated by: Bèr Kessels Status: patch -1 from me too. More hookss are unnecasary. What *is* neccesary, thoiugh, is to mve the "pre" to inbetween the title and the area! Bèr Kessels Previous comments: ------------------------------------------------------------------------ March 5, 2005 - 10:54 : jbond Attachment: http://drupal.org/files/issues/common.inc_8.patch (1.83 KB) This patch is for common.inc function form_textarea(). It adds two additional hooks for module writers. 1) A new hook "post-title" which works the same as "pre" and "post" but allows html to be inserted between the title of the element and the textarea. This makes it easier to insert a row of buttons (say) that sit just on top of the textarea. 2) A new hook "attributes" which takes an array() from modules that provide it and merges it into the $attributes array for the textarea. Wysywig editors typically need to add javascript handlers for things like onClick and this provides a simple way for the module to add them. ------------------------------------------------------------------------ March 5, 2005 - 11:00 : jbond Attachment: http://drupal.org/files/issues/common.inc_9.patch (1.83 KB) Apologies. the first patch had ms win line endings. This one should be unix line endings. ------------------------------------------------------------------------ March 6, 2005 - 10:17 : gordon -1 I do not think that these 2 additional hooks are needed. This will really add alot of additional overhead as it is going to call every textarea hook 4 times for every textarea on a page. Besides these modifiaction can be handled better and cleaner in the javascript. If you take a look at Xinha/HTMLArea or FckEditor they both manipulate the dom once the page has loaded, and not during. This is a much better and cleaner method as it means that you can attach to any textarea with only a few commands. This is what javascript excels at. So I don't think this is needed. ------------------------------------------------------------------------ March 6, 2005 - 11:03 : Dries I was about to suggest the same as Gordon. IMO, these hooks aren't really necessary.

1 0

[drupal-devel] CCK Nomenclature
by Jonathan Chaffer 06 Mar '05

06 Mar '05

Suppose node.module handles all of the content types defined by node modules as today, and a new module is added that handles the administrator-defined content types. What should that module be called? cck.module field.module type.module nodetype.module contenttype.module ... The chosen name will have an impact on the user interface to some degree, because in themes the content types will be prefixed by the name to distinguish them from content types defined by a node module. -- Jonathan Chaffer Applications Developer, structure:interactive (616) 364-7423 http://www.structureinteractive.com/

17 29

[drupal-devel] [bug] Putting a .theme file into themes/ makes all themes not working
by chx 06 Mar '05

06 Mar '05

Issue status update for http://drupal.org/node/16914 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: critical Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/themefix_6.patch (3.26 KB) Ah, you are proposing that we do not even call file_scan_directory when we are too deep? That's a wise idea, as usual from you ;) However, it can not be in the if-clause surrounding the call to file_scan_directory(), 'cos if you are expecting files, and hit max_depth, you may get some directories too. So it got a separate if. chx Previous comments: ------------------------------------------------------------------------ February 7, 2005 - 08:13 : chx Try putting a .theme file into themes/ without any subdir. Enable it (maybe just saving theme settings is enough), Nothing will work after this. To restore your site: remove the offending theme, issue an SQL: DELETE FROM {system} WHERE type='theme' and save theme settings again. ------------------------------------------------------------------------ February 7, 2005 - 08:14 : chx effects HEAD, too. ------------------------------------------------------------------------ February 7, 2005 - 13:33 : chx Attachment: http://drupal.org/files/issues/themesfix.patch (542 bytes) Here is a patch which fixes the problem. However, I do not have the feeling that this is "the" solution. Feel free to come up with something better. ------------------------------------------------------------------------ February 7, 2005 - 13:35 : chx Oops forgot to set to CVS, 'cos the fix is against HEAD. Not that 4.5.2 does not need a patch... ------------------------------------------------------------------------ February 7, 2005 - 22:15 : Anonymous While this patch would allow a theme to be placed directly in the "themes" directory, it disables the ability for that theme to use a "style.css" file, which is inconsistent. I would prefer a patch that enforces the criteria that themes MUST be placed in a SUBDIRECTORY of "themes" -- themes placed directly in the "themes" folder should be ignored... maybe we could send the admin a drupal_set_message that would tell them about the problem when visiting admin/themes too. ------------------------------------------------------------------------ February 23, 2005 - 00:07 : chx Attachment: http://drupal.org/files/issues/themefix.patch (693 bytes) Anonymous is right. I have made a better patch. Please commit, this is very important IMHO. ------------------------------------------------------------------------ February 23, 2005 - 00:30 : chx Attachment: http://drupal.org/files/issues/themefix_1.patch (712 bytes) Message was wrong. ------------------------------------------------------------------------ March 2, 2005 - 00:26 : chx May I know why this is not getting commited? This is a very nasty issue, the poor site admin who tries a few themes and misplaces one is totally baffled why his themes do not work after this. ------------------------------------------------------------------------ March 2, 2005 - 09:26 : Dries - That should probably be an error message, rather than a status (success) message. - The coding style needs work. - For consistency the theme name should be wrapped in em's. - I'd prefer not to scan ./themes for .theme files and to ignore any .theme files places directly in ./themes. Won't commit as is. ------------------------------------------------------------------------ March 2, 2005 - 12:37 : chx - That should probably be an error message, rather than a status (success) message. Ah, I was never aware of the second parameter of drupal_set_message! One always learns. - The coding style needs work. Spaces, my doom :( - For consistency the theme name should be wrapped in em's. Understood. - I'd prefer not to scan ./themes for .theme files and to ignore any .theme files places directly in ./themes. Well. Only a system_listing call happens before my patch in system_theme_data. All system_listing does it sets up a maximum of two starting points for a recursive file_scan_directory ($directory and $config/$directory) and file_scan_directory has no options for such an exclusion and it does not seem appropriate to change file_scan_directory only for this. Surely I miss something in here, but I still think that this special thing shall be done in system_theme_data. If this is accepted, the minor (from a coding point of view they are minor) issues will be corrected in no time. ------------------------------------------------------------------------ March 5, 2005 - 20:14 : chx Attachment: http://drupal.org/files/issues/themefix_2.patch (3.2 KB) This is a whole new approach. Some credits: to nsk, because he triggered the bug and bought me Shadow of The Giant for fixing his site, but I took that as a sponsoring to fix this bug. To Steven for the $depth-as-argument and to Dries 'cos of "sensible defaults". We do not throw warning this time 'cos system_theme_data is not even aware of the faulty themes. ------------------------------------------------------------------------ March 5, 2005 - 20:26 : chx Attachment: http://drupal.org/files/issues/themefix_3.patch (3.31 KB) PHPdoc and style fix ------------------------------------------------------------------------ March 5, 2005 - 20:48 : chx Attachment: http://drupal.org/files/issues/themefix_4.patch (3.29 KB) ------------------------------------------------------------------------ March 5, 2005 - 21:13 : chx Attachment: http://drupal.org/files/issues/themefix_5.patch (3.27 KB) ------------------------------------------------------------------------ March 6, 2005 - 11:12 : Dries Can't you perform the $depth <= $max_depth check before you call file_scan_directory()? Looks like you could move the check down to the if-clause surrounding the call to file_scan_directory(): <?php if (is_dir("$dir/$file") && $recurse) { ?>

1 0

[drupal-devel] [bug] Putting a .theme file into themes/ makes all themes not working
by Dries 06 Mar '05

06 Mar '05

Issue status update for http://drupal.org/node/16914 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: critical Assigned to: chx Reported by: chx Updated by: Dries Status: patch Can't you perform the $depth <= $max_depth check before you call file_scan_directory()? Looks like you could move the check down to the if-clause surrounding the call to file_scan_directory(): <?php if (is_dir("$dir/$file") && $recurse) { ?> Dries Previous comments: ------------------------------------------------------------------------ February 7, 2005 - 09:13 : chx Try putting a .theme file into themes/ without any subdir. Enable it (maybe just saving theme settings is enough), Nothing will work after this. To restore your site: remove the offending theme, issue an SQL: DELETE FROM {system} WHERE type='theme' and save theme settings again. ------------------------------------------------------------------------ February 7, 2005 - 09:14 : chx effects HEAD, too. ------------------------------------------------------------------------ February 7, 2005 - 14:33 : chx Attachment: http://drupal.org/files/issues/themesfix.patch (542 bytes) Here is a patch which fixes the problem. However, I do not have the feeling that this is "the" solution. Feel free to come up with something better. ------------------------------------------------------------------------ February 7, 2005 - 14:35 : chx Oops forgot to set to CVS, 'cos the fix is against HEAD. Not that 4.5.2 does not need a patch... ------------------------------------------------------------------------ February 7, 2005 - 23:15 : Anonymous While this patch would allow a theme to be placed directly in the "themes" directory, it disables the ability for that theme to use a "style.css" file, which is inconsistent. I would prefer a patch that enforces the criteria that themes MUST be placed in a SUBDIRECTORY of "themes" -- themes placed directly in the "themes" folder should be ignored... maybe we could send the admin a drupal_set_message that would tell them about the problem when visiting admin/themes too. ------------------------------------------------------------------------ February 23, 2005 - 01:07 : chx Attachment: http://drupal.org/files/issues/themefix.patch (693 bytes) Anonymous is right. I have made a better patch. Please commit, this is very important IMHO. ------------------------------------------------------------------------ February 23, 2005 - 01:30 : chx Attachment: http://drupal.org/files/issues/themefix_1.patch (712 bytes) Message was wrong. ------------------------------------------------------------------------ March 2, 2005 - 01:26 : chx May I know why this is not getting commited? This is a very nasty issue, the poor site admin who tries a few themes and misplaces one is totally baffled why his themes do not work after this. ------------------------------------------------------------------------ March 2, 2005 - 10:26 : Dries - That should probably be an error message, rather than a status (success) message. - The coding style needs work. - For consistency the theme name should be wrapped in em's. - I'd prefer not to scan ./themes for .theme files and to ignore any .theme files places directly in ./themes. Won't commit as is. ------------------------------------------------------------------------ March 2, 2005 - 13:37 : chx - That should probably be an error message, rather than a status (success) message. Ah, I was never aware of the second parameter of drupal_set_message! One always learns. - The coding style needs work. Spaces, my doom :( - For consistency the theme name should be wrapped in em's. Understood. - I'd prefer not to scan ./themes for .theme files and to ignore any .theme files places directly in ./themes. Well. Only a system_listing call happens before my patch in system_theme_data. All system_listing does it sets up a maximum of two starting points for a recursive file_scan_directory ($directory and $config/$directory) and file_scan_directory has no options for such an exclusion and it does not seem appropriate to change file_scan_directory only for this. Surely I miss something in here, but I still think that this special thing shall be done in system_theme_data. If this is accepted, the minor (from a coding point of view they are minor) issues will be corrected in no time. ------------------------------------------------------------------------ March 5, 2005 - 21:14 : chx Attachment: http://drupal.org/files/issues/themefix_2.patch (3.2 KB) This is a whole new approach. Some credits: to nsk, because he triggered the bug and bought me Shadow of The Giant for fixing his site, but I took that as a sponsoring to fix this bug. To Steven for the $depth-as-argument and to Dries 'cos of "sensible defaults". We do not throw warning this time 'cos system_theme_data is not even aware of the faulty themes. ------------------------------------------------------------------------ March 5, 2005 - 21:26 : chx Attachment: http://drupal.org/files/issues/themefix_3.patch (3.31 KB) PHPdoc and style fix ------------------------------------------------------------------------ March 5, 2005 - 21:48 : chx Attachment: http://drupal.org/files/issues/themefix_4.patch (3.29 KB) ------------------------------------------------------------------------ March 5, 2005 - 22:13 : chx Attachment: http://drupal.org/files/issues/themefix_5.patch (3.27 KB)

1 0

[drupal-devel] [bug] nodeapi 'settings' call sends string instead of object for $node value
by crunchywelch 06 Mar '05

06 Mar '05

Project: Drupal Version: cvs Component: node system Category: bug reports Priority: normal Assigned to: crunchywelch Reported by: crunchywelch Updated by: crunchywelch Status: patch Attachment: http://drupal.org/files/issues/node_24.patch (1.42 KB) when the 'settings' nodeapi operation is invoked, a string containing the type of node is sent for the $node argument, instead of an object like the rest of the operations. This breaks comment.module, and is inconsistent with the rest of the operations and documentation. This small patch fixes the problem. crunchywelch -- View: http://drupal.org/node/16384 Edit: http://drupal.org/project/comments/add/16384

4 4

[drupal-devel] [feature] 2 additional hooks to form_textarea to support module writers who are extending it.
by Dries 06 Mar '05

06 Mar '05

Issue status update for http://drupal.org/node/18417 Project: Drupal Version: 4.5.2 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: jbond Updated by: Dries Status: patch I was about to suggest the same as Gordon. IMO, these hooks aren't really necessary. Dries Previous comments: ------------------------------------------------------------------------ March 5, 2005 - 11:54 : jbond Attachment: http://drupal.org/files/issues/common.inc_8.patch (1.83 KB) This patch is for common.inc function form_textarea(). It adds two additional hooks for module writers. 1) A new hook "post-title" which works the same as "pre" and "post" but allows html to be inserted between the title of the element and the textarea. This makes it easier to insert a row of buttons (say) that sit just on top of the textarea. 2) A new hook "attributes" which takes an array() from modules that provide it and merges it into the $attributes array for the textarea. Wysywig editors typically need to add javascript handlers for things like onClick and this provides a simple way for the module to add them. ------------------------------------------------------------------------ March 5, 2005 - 12:00 : jbond Attachment: http://drupal.org/files/issues/common.inc_9.patch (1.83 KB) Apologies. the first patch had ms win line endings. This one should be unix line endings. ------------------------------------------------------------------------ March 6, 2005 - 11:17 : gordon -1 I do not think that these 2 additional hooks are needed. This will really add alot of additional overhead as it is going to call every textarea hook 4 times for every textarea on a page. Besides these modifiaction can be handled better and cleaner in the javascript. If you take a look at Xinha/HTMLArea or FckEditor they both manipulate the dom once the page has loaded, and not during. This is a much better and cleaner method as it means that you can attach to any textarea with only a few commands. This is what javascript excels at. So I don't think this is needed.

1 0

[drupal-devel] [feature] 2 additional hooks to form_textarea to support module writers who are extending it.
by jbond 06 Mar '05

06 Mar '05

Issue status update for http://drupal.org/node/18417 Project: Drupal Version: 4.5.2 Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: jbond Updated by: jbond -Status: active +Status: patch Attachment: http://drupal.org/files/issues/common.inc_9.patch (1.83 KB) Apologies. the first patch had ms win line endings. This one should be unix line endings. jbond Previous comments: ------------------------------------------------------------------------ March 5, 2005 - 09:54 : jbond Attachment: http://drupal.org/files/issues/common.inc_8.patch (1.83 KB) This patch is for common.inc function form_textarea(). It adds two additional hooks for module writers. 1) A new hook "post-title" which works the same as "pre" and "post" but allows html to be inserted between the title of the element and the textarea. This makes it easier to insert a row of buttons (say) that sit just on top of the textarea. 2) A new hook "attributes" which takes an array() from modules that provide it and merges it into the $attributes array for the textarea. Wysywig editors typically need to add javascript handlers for things like onClick and this provides a simple way for the module to add them.

2 1

[drupal-devel] CCK Node Caching Strategies
by Jonathan Chaffer 06 Mar '05

06 Mar '05

The CCK implementation is going to require a resurrection of node caching, but I'm considering some different approaches. This is an attempt to document the design process, while at the same time soliciting suggestions. For this analysis, assume that each content type has 10 simple text fields, as well as a multiple-entry text field with an average of 10 entries. We will be looking at the queries necessary to load and display a listing of 10 of these nodes. One approach is to attempt to join the tables together, and load all the fields in one query. This can work in general (Flexinode does this), but has a few drawbacks. First, the JOIN operation is slow (though faster than doing the queries separately). Second, there is a hard limit of 31 joins in a query for MySQL, so this prevents complex content types from being constructed. Some users have hit this limit when using Flexinode. Third, the multiple-entry field would have to be serialized for this to work, which has a significant impact on its flexibility when performing queries. 1x query to fetch node IDs for the ten nodes (10 results) 10x queries to fetch each node, each with 11 joins (1 result each, + deserialize) The joins can't be performed in the original query, because the content types could be different in each case. A simpler implementation is to load each field through a separate query. This is a very flexible approach, and allows simple compartmentalization of the query code into the field modules. The obvious downside is that you need many queries for a simple node load. 1x query to fetch node IDs for the ten nodes (10 results) 100x queries to fetch each normal field (1 result each) 10x queries to fetch the multi-value fields (10 results each) Next, we can add caching to this, the way Gerhard proposed. This means that in node_load(), a query is performed to the cache table to load the serialized node data. 1x query to fetch node IDs for the ten nodes (10 results) 10x queries to fetch each node (1 result each, + deserialize) I think we can do a bit better, though. By placing the node cache into a column in the node table, we can grab it at the same time as the node IDs. Then node_load() would look to see if the cache is present in the array presented to it, and if so use that rather than perform any queries at all. 1x query to fetch node IDs and cache for the ten nodes (10 results + deserialize) Note that we can use this method with the cache in the cache table as well, but that requires a JOIN using a CONCAT() in the ON clause, which seems risky, and involves direct knowledge of the cache table structure rather than using the cache API anyway. Any other ideas? -- Jonathan Chaffer Applications Developer, structure:interactive (616) 364-7423 http://www.structureinteractive.com/

3 2

[drupal-devel] [bug] Consider the '$' as part of a valid URL
by mathias 06 Mar '05

06 Mar '05

Issue status update for http://drupal.org/node/18437 Project: Drupal Version: cvs Component: other Category: bug reports Priority: normal Assigned to: mathias Reported by: mathias Updated by: mathias Status: patch Attachment: http://drupal.org/files/issues/valid-url.patch (810 bytes) Drupal doesn't allow URL aliases that map to Userland Manila posts since they usually contain the '$' and are considered an invalid URL. This patch allows '$' in an URL and thus an alias. It also resolves a disparity between the 'allowable characters' of absolute and relative URLs. As far as I can tell, those parts of the regexp should be the same. mathias

2 1

Re: [drupal-devel] forum.module dependent on comment.module
by Boris Mann 06 Mar '05

06 Mar '05

On Fri, 04 Mar 2005 19:33:43 -0600, Mark <mark(a)nullcraft.org> wrote: > There has been discussion about implementing comments as nodes. What > would the difficulties be with such a scenario? If comments were > nodes, then it would be trivial to specify additional node types that > could be used as comments (files and images, anyone?). I think if > relativity module were simplified and a lot of the more advanced > features were moved into separate module, just leaving the relationship > management part, this would be quite feasible. > > I'm curious to see what others think about such a scenario. I tried to steer away from the comments-as-nodes discussion, but you seem to have stumbled right into it :P It basically means some major rewriting, and there is value in keeping comments as a separate content type, I think. I'm not willing to dive into this can of worms right now. -- Boris Mann http://www.bryght.com

7 12