Moodle

tag/lib.php issues

Details

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

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 (skodak) 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 (skodak) 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 (skodak) added a comment -

thanks!

Show
Petr Škoda (skodak) added a comment - thanks!
Hide
Petr Škoda (skodak) added a comment -

reopening - direct using of mb_string extension is not acceptable.

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

reclosing

Show
Petr Škoda (skodak) added a comment - reclosing

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: