Looking for patch love
Hello world, there is a patch that attempts to clean up the node submit hooks: http://drupal.org/node/68418 Simarly, check out the profile rendering changes at: http://drupal.org/node/79227 It seems to make for a new API trend so we better take the time to think this through. Personally, I've mixed feelings about it so I figured I'd bring this up on the mailing list. Here is what I think: Advantages: - No disconnect between the regular flow (through forms) and a programatic flow. Less prone to errors, improved APIs to do thing programmatically. Disadvantages: - Improved consistency, but not ultimate consistency. Most things will continue to use the "old style" worfklow (and hook mechanism). - Not always easier to use or easier to theme. Not exactly elegant either -- often very verbose. - Expensive in terms of processing cost. Performance matters a lot and unless we make Drupal faster, we won't see bigger and bigger Drupal site. Let's weight the advantages and disadvantages, and lets figure out some practical use cases. When discussing, stick to the topic at hand and avoid wandering off to Meta Land to discuss the way we do things in general, or how we should do things. This is a _technical_ mailing list, and I'm explicitly asking for a _technical_ discussion on this matter. Don't bother responding unless you spent at least 20 minutes poking at the above patches, or unless you tried an example or two. Take the time to investigate this, be critic, and share your own opinion rather than repeating what others have said (without having looked at the patches yourself). There, I've said it. :) -- Dries Buytaert :: http://www.buytaert.net/
Hi We seem to be moving towards an overall new hook system, all based around the callbacks and alters of forms. I have worked with form api for a long enough time to say that I very much dislike it. I prefer hooks over _alter's and callbacks. Thats all.
- Not always easier to use or easier to theme. Not exactly elegant either -- often very verbose.
Theme functions in fapi have become a LOT more complex. EG: +function theme_profile_category($element) { + $output = ($element['#title'] != '') ? '<h3>'. $element['#title'] .'</h3>' : ''; + $output .= '<dl'. drupal_attributes($element['#attributes']) .'>'; + + foreach (element_children($element) as $key) { + if ($element[$key]['#title'] != '') { + $output .= '<dt'. drupal_attributes($element[$key] ['#attributes']) .'>'. $element[$key]['#title'] .'</dt>'; + unset($element[$key]['#title']); + $output .= '<dd'. drupal_attributes($element[$key] ['#attributes']) .'>' . $element[$key]['#value'] .'</dd>'; + unset($element[$key]['#value']); + } + } + $output .= '</dl>'; + return $output; +} 2 ifs, 1 foreach, 1 unset, 2 api calls and loads of complex concanations. This should not even be considered a theme function. This is business logic pur sang. Complex business logic even. Not fit for a theme. FAPI (alike code) has introduced lots of these waaaay too complex theme functions. Compare to a properly crafted theme function, wich gets prepared data: function theme_profile_category($elements) { foreach ($elements as $key => $element) { $out .= theme('profile_element', $element, $key); } return "<dl>$out</dl>"; } function theme_profile_element($element, $key) { $elements = drupal_attributes($element['#attributes']); $out .= "<dt $elements>$element['#title']</dt>"; $out .= "<dd>$element['#value']</dd>" } This latter solution is 1) cleaner, 2) gives the same power to the themer, and is 3) far easier to grok. I don't have a good feeling about this FAPI direction. not at all. Is this explanation why i dislike the fapi-concept technical enough? There, I've said it. :) Bèr -- PGP ber@webschuur.com http://www.webschuur.com/sites/webschuur.com/files/ber_webschuur.asc Nieuwsartikelen, of weblog.: http://help.sympal.nl/nieuwsartikelen_of_weblog
There, I've said it. :)
all true, but it isn't on topic. the easiest way to theme the user profile in this patch is to edit user-profile.tpl.php. the whole $account object is available to you, including the $account->content object which is now a structured array of fields and categories, just like before. the fields that are available in that file will be documented at http://drupal.org/phptemplate, as is our custom. user.tpl.php doesn't even exist before this patch, and alone represents a substantial themeing advance.
On 26 Aug 2006, at 20:12, Moshe Weitzman wrote:
the easiest way to theme the user profile in this patch is to edit user-profile.tpl.php. the whole $account object is available to you, including the $account->content object which is now a structured array of fields and categories, just like before. the fields that are available in that file will be documented at http:// drupal.org/phptemplate, as is our custom.
But from the looks, Ber's proposed theme functions are easier ... can we make it exactly like Ber proposed with the new system or would it get crufty? :-) -- Dries Buytaert :: http://www.buytaert.net/
Op dinsdag 29 augustus 2006 08:46, schreef Dries Buytaert:
But from the looks, Ber's proposed theme functions are easier ... can we make it exactly like Ber proposed with the new system or would it get crufty? :-)
On the other hand, I don't really mind (ahum) cleaning up theme functions into improved/consistent/easy to use functions either. Eaton managed to do quite a few last release cycle, and I have lots of them in the issue queue waiting for (me to do) some work. I guess its an uphill battle, but I guess its a battle we might finally win. I relialized that a) developers are not themers. They care about funcitonality, performance, good code and features. Not about how easy it is to make their (perfect) baby look different. This is good. Lets try to do what we are best at. b) people who do both coding and *good* theming are so rare that they all got "bought" by Bryght last month ... :). Eaton is probably the only person who had the time, and spirit to keep up all those theme improvement patches up with HEAD towards 4.8, we owe him some fabulous theme improvements. e) most themers are just a bunch of whining babies. They fail to write proper technical patches, in complex code refacturings. So all they do is write a) b), c)s up to e)s to be heard. Most of their coding skills are below average, so helping out in these complex patches is hard, for them, but writing meta-mails is all they can do to get heard when their area seems under attack/attention somehow. Solution: So far the only reasonable is: Don't mind too much about themeability. These patches will slowly make it in, in a next (or next after next) release.(Provided Eaton does not leave us soon, that is) But serious: Kika (Kristjan) proposed this a while ago, in Amsterdam, wrt usability, but it can be extended to themeing too: Patches, issues etc should be reviewed by a flock of people. People whom mostly cannot read patches. People who want to see stuff like screenshots, or example HTML. Stuff like 'this was how you themed this function before, this is what you should do now'. But other then that -developers putting effort in getting a coloured flock of reviewers- we could make this is a little more "official". I have brought this up before, and will continue doing so: We have coding guidelines telling where a { should go, how many spaces one should put between a . and a $ etc. But we have nothing, probably not even a clue, what a good theme function looks like, or what IDs and CLASSES one should use. What would it take to get that off the ground? To get some regulation, some guidelines on what is themable and what not Bèr -no, i am not entirely serious- Kessels
Op dinsdag 29 augustus 2006 08:46, schreef Dries Buytaert:
But from the looks, Ber's proposed theme functions are easier ... can we make it exactly like Ber proposed with the new system or would it get crufty? :-)
Did anyone notice my DL patch? I modeled it after the theme_item_list exactly, both in use of XHTML (wrapper divs), classes and IDs. I suspect that this user-profile patch could be simplified a lot, if we (I?) worked on that theme_definition_list() patch parallel, so that the user-profile patch can focus on what it should do: improve the profile code. its here: http://drupal.org/node/54898 (status: code needs work, I am the one to blame, sorry) Bèr
like dries, i'm not 100% certain that these patches point us in the precise direction that we want to go. but i do think they move us significantly forward, which is about all we ask from a patch. none of us has certainty about where this is all headed. if we don't like working with these flows, we can change the code for the following release. this is not a marriage.
Disadvantages: - Improved consistency, but not ultimate consistency. Most things will continue to use the "old style" worfklow (and hook mechanism).
IMO, this is not a real disadvantage. We never had ultimate consistency, and never will. We will keep improving "old style" stuff as itches get scratched. The latest version of the node submit patch lets node modules keep their their hook paradigm. the node.module is registering fapi callbacks for them. just the function names and params changed slightly.
- Not always easier to use or easier to theme. Not exactly elegant either -- often very verbose.
themeing pertains only to the profile patch. as mentioned elsewhere, i expect people to theme using the new user_profile.tpl.php. i agree that theme_user_profile is more complicated than before, but thats not really important .
- Expensive in terms of processing cost. Performance matters a lot and unless we make Drupal faster, we won't see bigger and bigger Drupal site.
yes, performance matters a lot. it is less relevant for the node submit patch, since node submission is a rare event. node submit uses standard fapi, so unless we want to open a debate on fapi performance, this is off topic. note that most sites do show at least 2 forms every page view (search and login), and thats a where i suggest we pour attention if we care about fapi performance. the performance of the user profile patch is more interesting. constructing the various fields and categories is virtually unchanged. the rendering is a bit more complex in this patch, but we already tested this with the node body patch (http://drupal.org/node/74326). this is perfectly analogous, except for the fact that it happens far less frequently (and thus is less risky). do we really have to benchmark again? this is just a different array going through the same drupal_render() engine. SUMMARY i think i've addressed most of the concerns. i'm willing to keep doing so. users can still submit nodes and view profiles. with this patch developers can programmatically submit nodes easier, and can view/alter profiles using a more standardized method. they can alter profile fields to show/hide using #access, and they can recategorize as needed. themers get a new user_profile.tpl.php, and well as cleaned up markup in case they want to style using only css.
When discussing, stick to the topic at hand and avoid wandering off to Meta Land to discuss the way we do things in general, or how we should do things. This is a _technical_ mailing list, and I'm explicitly asking for a _technical_ discussion on this matter. Don't bother responding unless you spent at least 20 minutes poking at the above patches, or unless you tried an example or two. Take the time to investigate this, be critic, and share your own opinion rather than repeating what others have said (without having looked at the patches yourself). There, I've said it. :)
this is a terrific thread starter. i'd like to append it to all my issues.
participants (3)
-
Bèr Kessels -
Dries Buytaert -
Moshe Weitzman