Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9
    • Component/s: Tags
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      28655

      Description

      1/ do not include config.php
      2/ do not use mb_string functions directly - use our textlib instead
      3/ do not do capability checks in library functions in lib.php - do them before calling the tag_create() etc. (it is ok for html printing function)
      4/ untag_an_item() - uninitialised $norm_tag_names_or_ids_csv
      5/ why stripcslashes() in correlated_tags() ?
      6/ html printing function should have $return option to return string instead of printing it

      7/ compare tag_normalize() with clean_filename() - these could IMHO use similar code, tag_normalize() does not remove all control chars which is IMHO wrong
      8/ use unicode-aware string function from textlib - example: substr with MAX_TAG_LENGTH parameter does not work as expected for multibyte tags

        Issue Links

          Activity

          Hide
          Luiz Cruz added a comment -

          1/ do not include config.php
          Removed it...

          2/ do not use mb_string functions directly - use our textlib instead
          Using textlib where possible.
          I wasnt able to find a substitute for mb_convert_case() in textlib though

          3/ do not do capability checks in library functions in lib.php - do them before calling the tag_create() etc. (it is ok for html printing function)

          I thought a bit about this before adding capability checking in tag_create. My conclusions were

          • Introducing a capability check directly in the tag API increases its coupling to the the capability system (pretty bad..)
          • However, expecting moodle programmers to remember everytime to check if a user has the capability to create a tag is reaaally bad. With certainty, someone will forget somewhere to add the check and we will have a bug. In addition, there some other functions that implicitly create new tags, such as tag_an_item() and update_item_tags().
          • tag_create is the one centralized function of the API responsible for creating new tags. If everytime you create a tag you need to check if the user has this capability, then tag_create() is the perfect place to do it.
          • If a more "direct acess" to tags is needed... Just use the dmllib.php function.

          4/ untag_an_item() - uninitialised $norm_tag_names_or_ids_csv
          Fixed it.

          5/ why stripcslashes() in correlated_tags() ?
          Actually, I´m not sure

          6/ html printing function should have $return option to return string instead of printing it
          Fixed it

          7/ compare tag_normalize() with clean_filename() - these could IMHO use similar code, tag_normalize() does not remove all control chars which is IMHO wrong
          Maybe tag_normalize() needs to be updated to avoid control chars. I took a look at clean_filename(), but it seems that it only allows ascii chars (not sure...). I really don´t know if they have the same functionality.

          8/ use unicode-aware string function from textlib - example: substr with MAX_TAG_LENGTH parameter does not work as expected for multibyte tags
          Fixed it

          Show
          Luiz Cruz added a comment - 1/ do not include config.php Removed it... 2/ do not use mb_string functions directly - use our textlib instead Using textlib where possible. I wasnt able to find a substitute for mb_convert_case() in textlib though 3/ do not do capability checks in library functions in lib.php - do them before calling the tag_create() etc. (it is ok for html printing function) I thought a bit about this before adding capability checking in tag_create. My conclusions were Introducing a capability check directly in the tag API increases its coupling to the the capability system (pretty bad..) However, expecting moodle programmers to remember everytime to check if a user has the capability to create a tag is reaaally bad. With certainty, someone will forget somewhere to add the check and we will have a bug. In addition, there some other functions that implicitly create new tags, such as tag_an_item() and update_item_tags(). tag_create is the one centralized function of the API responsible for creating new tags. If everytime you create a tag you need to check if the user has this capability, then tag_create() is the perfect place to do it. If a more "direct acess" to tags is needed... Just use the dmllib.php function. 4/ untag_an_item() - uninitialised $norm_tag_names_or_ids_csv Fixed it. 5/ why stripcslashes() in correlated_tags() ? Actually, I´m not sure 6/ html printing function should have $return option to return string instead of printing it Fixed it 7/ compare tag_normalize() with clean_filename() - these could IMHO use similar code, tag_normalize() does not remove all control chars which is IMHO wrong Maybe tag_normalize() needs to be updated to avoid control chars. I took a look at clean_filename(), but it seems that it only allows ascii chars (not sure...). I really don´t know if they have the same functionality. 8/ use unicode-aware string function from textlib - example: substr with MAX_TAG_LENGTH parameter does not work as expected for multibyte tags Fixed it
          Hide
          Petr Škoda added a comment -

          if you do not find a function in textlib then first try typo3 that is used internally, we can add new functions there - please file a separate bug if needed

          the clean_filename() either converts to ascii or cleans only the ascii range - see the long regex with ranges there.

          Show
          Petr Škoda added a comment - if you do not find a function in textlib then first try typo3 that is used internally, we can add new functions there - please file a separate bug if needed the clean_filename() either converts to ascii or cleans only the ascii range - see the long regex with ranges there.
          Hide
          Petr Škoda added a comment -

          thanks!

          Show
          Petr Škoda added a comment - thanks!
          Hide
          Petr Škoda added a comment -

          reopening - direct using of mb_string extension is not acceptable.

          Show
          Petr Škoda added a comment - reopening - direct using of mb_string extension is not acceptable.
          Hide
          Petr Škoda added a comment -

          reclosing

          Show
          Petr Škoda added a comment - reclosing

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: