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

      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

          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 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
            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
            Petr Skoda added a comment -

            thanks!

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

            reopening - direct using of mb_string extension is not acceptable.

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

            reclosing

            Show
            Petr Skoda added a comment - reclosing

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: