MDL-21198 improved html_link api, added missing style attribute; mark…

…ed some potential problems with TODOs

lib/outputcomponents.php

@@ -1269,9 +1269,9 @@ class html_link extends html_component {
     public $url;
 
     /**
-     * @var string $text The text that will appear between the link tags
+     * @var string $text The HTML text that will appear between the link tags
      */
-    public $text;
+    public $text = '';
 
     /**
      * @var boolean $disabled Whether or not this link is disabled (will be rendered as plain text)
@@ -1283,23 +1283,49 @@ class html_link extends html_component {
      */
     public $disableifcurrent = false;
 
+    /**
+     * New link constructor.
+     *
+     * @param moodle_url|string $url url of the image
+     * @param array $options link attributes such as title, id, disabled, disableifcurrent, etc.
+     */
+    public function __construct($url = null, $text = '', array $options = null) {
+        parent::__construct($options);
+
+        if (is_null($url)) {
+            // to be filled later
+
+        } else if ($url instanceof moodle_url) {
+            $this->src = clone($url);
+
+        } else if (is_string($url)) {
+            $this->src = new moodle_url($url);
+
+        } else {
+            throw new coding_style_exception('Image can be constructed only from moodle_url or string url.');
+        }
+
+        $this->text = $text;
+    }
+
     /**
      * @see lib/html_component#prepare() Disables the link if it links to the current page.
      * @return void
      */
     public function prepare(renderer_base $output, moodle_page $page, $target) {
         // We can't accept an empty text value
-        if (empty($this->text)) {
+        if ($this->text === '' or is_null($this->text)) { // 0 is valid value, do not use empty()
             throw new coding_exception('A html_link must have a descriptive text value!');
         }
 
         if (!($this->url instanceof moodle_url)) {
             $this->url = new moodle_url($this->url);
         }
 
-        if ($this->url->compare($page->url, URL_MATCH_PARAMS) && $this->disableifcurrent) {
+        if ($this->disableifcurrent and $this->url->compare($page->url, URL_MATCH_PARAMS)) {
             $this->disabled = true;
         }
+
         parent::prepare($output, $page, $target);
     }
 
@@ -1310,11 +1336,7 @@ public function prepare(renderer_base $output, moodle_page $page, $target) {
      * @return html_link The link component
      */
     public static function make($url, $text) {
-        $link = new html_link();
-        $link->url = $url;
-        $link->text = $text;
-
-        return $link;
+        return new html_link($url, $text);
     }
 }
 

lib/outputrenderers.php

@@ -822,48 +822,40 @@ public function block_move_target($target) {
      * Given a html_link object, outputs an <a> tag that uses the object's attributes.
      *
      * @param mixed $link A html_link object or a string URL (text param required in second case)
-     * @param string $text A descriptive text for the link. If $link is a html_link, this is not required.
+     * @param string $text A descriptive text for the link. If $link is a html_link, this is ignored.
+     * @param array $options a tag attributes and link otpions. If $link is a html_link, this is ignored.
      * @return string HTML fragment
      */
-    public function link($link, $text=null) {
+    public function link($link_or_url, $text = null, array $options = null) {
         global $CFG;
 
-        $attributes = array();
-
-        if (is_a($link, 'html_link')) {
-            $link = clone($link);
-
-            if ($link->has_action('popup_action')) {
-                return $this->link_to_popup($link);
-            }
-
-            $link->prepare($this, $this->page, $this->target);
-            $this->prepare_event_handlers($link);
-
-            // A disabled link is rendered as formatted text
-            if ($link->disabled) {
-                return $this->container($link->text, 'currentlink');
-            }
+        if ($link_or_url instanceof html_link) {
+            $link = clone($link_or_url);
+        } else {
+            $link = new html_link($link_or_url, $text, $options);
+        }
 
-            $attributes['href'] = prepare_url($link->url);
-            $attributes['class'] = $link->get_classes_string();
-            $attributes['title'] = $link->title;
-            $attributes['id'] = $link->id;
+        $link->prepare($this, $this->page, $this->target);
 
-            $text = $link->text;
+        // A disabled link is rendered as formatted text
+        if ($link->disabled) {
+            return $this->container($link->text, 'currentlink');
+        }
 
-        } else if (empty($text)) {
-            throw new coding_exception('$OUTPUT->link() must have a string as second parameter if the first param ($link) is a string');
+        $this->prepare_event_handlers($link);
 
-        } else {
-            $attributes['href'] = prepare_url($link);
-        }
+        $attributes = array('href'  => prepare_url($link->url),
+                            'class' => $link->get_classes_string(),
+                            'title' => $link->title,
+                            'style' => $link->style,
+                            'id'    => $link->id);
 
         if (!empty($CFG->frametarget)) {
+            //TODO: this seems wrong, we have to use onclick hack in order to be xhtml strict...
             $attributes['target'] = $CFG->framename;
         }
 
-        return $this->output_tag('a', $attributes, $text);
+        return $this->output_tag('a', $attributes, $link->text);
     }
 
    /**
@@ -1100,6 +1092,8 @@ public function help_icon($icon) {
      * @return string HTML fragment
      */
     public function link_to_popup($link, $image=null) {
+        //TODO: decide if this should be removed completely, because link() already handles this
+        //      we could also add html_link::make_popup() factory method
         $link = clone($link);
 
         // Use image if one is given
@@ -1112,7 +1106,7 @@ public function link_to_popup($link, $image=null) {
 
             $link->text = $this->image($image);
 
-            if (!empty($link->linktext)) {
+            if (!empty($link->linktext)) { // TODO: this is weird!
                 $link->text = "$link->title &#160; $link->text";
             }
         }