Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              luiz.laydner 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.laydner 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              skodak Petr Skoda added a comment -

              thanks!

              Show
              skodak Petr Skoda added a comment - thanks!
              Hide
              skodak Petr Skoda added a comment -

              reopening - direct using of mb_string extension is not acceptable.

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

              reclosing

              Show
              skodak Petr Skoda added a comment - reclosing

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Mar/08