drupal_strlen()
drupal_substr()

Via #578520: Make $query in url() only accept an array:

drupal_rawurlencode()
drupal_http_build_query()

And a lot more.

We want to add a doxygen group 'php_wrappers' and add a @ingroup directive to all those functions.

So developers new to Drupal can look up that list and start to memorize which of the regular PHP functions they should not use.

CommentFileSizeAuthor
#10 584966-Doxygen-php-wrappers-6.patch464 bytesmr.baileys
#6 584966-Doxygen-php-wrappers-5.patch7.64 KBmr.baileys
#4 584966-Doxygen-php-wrappers-4.patch7.67 KBmr.baileys
#2 584966-Doxygen-php-wrappers.patch6.9 KBmr.baileys
#1 drupal.php-wrappers.patch1.34 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
StatusFileSize
new drupal.php-wrappers.patch1.34 KB

Starting point.

Anyone wanting to take over from here?

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
StatusFileSize
new 584966-Doxygen-php-wrappers.patch6.9 KB

First pass:

  1. Really strict interpretation: all instances where the function name starts with drupal_ or php_ followed by the name of a function that exists in PHP have now been added to the "php_wrappers" group. This excludes functions like drupal_encode_path, drupal_session_commit (even if this is a wrapper around some other php session related functions) since these aren't php wrapper functions in the strict sense.
  2. Patch does not take into account non drupal_/php_ wrappers in optional core modules such as (for example) image_gd_rotate. Should it?
  3. Not sure about drupal_mail: Indirectly, it does wrap around php's mail (unless the MailSystemInterface implementation uses a different means to send the emails), but it does so much more than just wrap, and it doesn't call mail directly. I left it out for now, but since it should probably be called instead of mail we might want to include it...
  4. The example functions given in the original post (drupal_rawurlencode() and drupal_http_build_query()) don't exist yet so are probably part of non-commited patches, and the directive will have to be added in those patches or after those patches get commited.
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Awesome start!

+++ includes/common.inc	24 Sep 2009 07:20:11 -0000
@@ -10,6 +10,37 @@
+ * For ease of use and memorizing, all these wrapper functions use the same name
+ * as the original PHP function, but prefixed with "drupal_" or "php_". Beware,
+++ modules/php/php.module	24 Sep 2009 07:20:12 -0000
@@ -48,6 +48,8 @@
 function php_eval($code) {

php_eval() is a wonky exception, because the functions belongs to php.module. The PHPDoc shouldn't mention this when explaining the pattern.

+++ includes/common.inc	24 Sep 2009 07:20:11 -0000
@@ -4326,7 +4359,7 @@
 function drupal_render_cache_set($markup, $elements) {
-  // Create the cache ID for the element 
+  // Create the cache ID for the element

Either leave those white-space clean-ups out - or - add a trailing period (full-stop) to the comment here.

+++ includes/unicode.inc	24 Sep 2009 07:20:12 -0000
@@ -391,6 +393,11 @@
 /**
+ * @ingroup php_wrappers
+ * @{
+ */
@@ -440,14 +447,6 @@
 /**
- * Helper function for case conversion of Latin-1.
- * Used for flipping U+C0-U+DE to U+E0-U+FD and back.
- */
-function _unicode_caseflip($matches) {
-  return $matches[0][0] . chr(ord($matches[0][1]) ^ 32);
-}
@@ -546,4 +545,15 @@
+/**
+ * @} End of "ingroup php_wrappers".
+ */
+
+/**
+ * Helper function for case conversion of Latin-1.
+ * Used for flipping U+C0-U+DE to U+E0-U+FD and back.
+ */
+function _unicode_caseflip($matches) {
+  return $matches[0][0] . chr(ord($matches[0][1]) ^ 32);
+}

Can we please add @ingroup to the individual functions instead and revert the moving of that function?

I'm on crack. Are you, too?

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
StatusFileSize
new 584966-Doxygen-php-wrappers-4.patch7.67 KB

Thanks for the review, new patch attached which takes the comments from #3 into account.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Awesome! This is almost ready to fly!

+++ includes/unicode.inc	28 Sep 2009 06:43:58 -0000
@@ -544,6 +556,4 @@
-}
-
-
+}
\ No newline at end of file

Please revert this - there always needs to be at least one newline at the end of a file.

I'm on crack. Are you, too?

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
StatusFileSize
new 584966-Doxygen-php-wrappers-5.patch7.64 KB

I've put the newline back where I found it :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Great job!

Though, in general, try to avoid changes in code that you are not really touching. It 1) increases the possibility that your own patch will break when another is committed and 2) breaks a lot more patches in the current core queue for no good reason.

dries’s picture

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

Looks good. Committed to CVS HEAD. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Holy!

I have to say, this totally rocks:

http://api.drupal.org/api/group/php_wrappers/7

:)

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Fixed » Needs review
StatusFileSize
new 584966-Doxygen-php-wrappers-6.patch464 bytes

Small fix for the 'End of "defgroup php_wrappers".' that is showing up on http://api.drupal.org/api/group/php_wrappers/7

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

Thanks!

Actually, I used the "forms" group in form.inc as template. It contains that same output error: http://api.drupal.org/api/group/forms/7

The point is though that these

@} End of "defgroup php_wrappers".

statements

1) belong to the official/original doxygen syntax, but the API parser doesn't seem to understand that.

2) shouldn't even be contained in the output if the API parser would properly interpret the closing @}, because that is the hard limit of which code belongs into the group and which code not.

So either 1) or 2) would fix that issue. However, both are API parser issues and we really should fix the parser instead of the code, so I'm reverting the status of this issue to fixed.

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -API clean-up

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

Issue tags: +Novice, +API clean-up

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.