Moodle

Improve tagging system

Details

  • Type: Task Task
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: 1.9, 2.0
  • Fix Version/s: None
  • Component/s: Tags
  • Labels:
    None
  • Affected Branches:
    MOODLE_19_STABLE, MOODLE_20_STABLE

Description

Tag system needs a few improvements

  1. taglib.php
    21/Feb/08 6:18 PM
    38 kB
    Mathieu Petit-Clair
  2. tags_undo_HEAD.patch
    23/Feb/08 5:08 AM
    48 kB
    Petr Škoda (skodak)
  3. tags_undo.patch
    23/Feb/08 5:09 AM
    123 kB
    Petr Škoda (skodak)

Issue Links

Progress
Resolved Sub-Tasks Unresolved Sub-Tasks

Sub-Tasks

Activity

Hide
Mathieu Petit-Clair added a comment -

I just attached the file "tag/taglib.php" to this bug, and sent a big patch to http://moodle.pastebin.com/m3849f2a5 ... These two things together should apply to 19_STABLE (as of 21/02).

Many fixes, changes and probably a few bugs still.

Show
Mathieu Petit-Clair added a comment - I just attached the file "tag/taglib.php" to this bug, and sent a big patch to http://moodle.pastebin.com/m3849f2a5 ... These two things together should apply to 19_STABLE (as of 21/02). Many fixes, changes and probably a few bugs still.
Hide
Mathieu Petit-Clair added a comment -

I was in a hurry last night, so here's a bit more information about the patch... It solves the "tag cron error" and the "correlation sql error" and it makes it possible to use any character in a tag (letters, numbers, symbols, anything unicode).

It also removes a bit of code in a few files, using tag_* functions instead.

Show
Mathieu Petit-Clair added a comment - I was in a hurry last night, so here's a bit more information about the patch... It solves the "tag cron error" and the "correlation sql error" and it makes it possible to use any character in a tag (letters, numbers, symbols, anything unicode). It also removes a bit of code in a few files, using tag_* functions instead.
Hide
Eloy Lafuente (stronk7) added a comment -

Hi Mathieu,

yesterday I was looking to the pastebin code above and I found it quite interesting, causing more clean and readable code, for sure. B-)

Anyway one think I personally disagree y the use of arrays to pass parameters to functions and I've seen some of them in your code. Wouldn't it better to use "standard" multiple parameters instead of relying in arrays of them?

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Hi Mathieu, yesterday I was looking to the pastebin code above and I found it quite interesting, causing more clean and readable code, for sure. B-) Anyway one think I personally disagree y the use of arrays to pass parameters to functions and I've seen some of them in your code. Wouldn't it better to use "standard" multiple parameters instead of relying in arrays of them? Ciao
Hide
Eloy Lafuente (stronk7) added a comment -

And another, really minor comment is about WHERE the taglib.php should be? Within /tag ? Or within /lib, leaving /tag for interface and so on... uhm... not critical at all but I love to have libraries in /lib. For your consideration.

Ciao

Show
Eloy Lafuente (stronk7) added a comment - And another, really minor comment is about WHERE the taglib.php should be? Within /tag ? Or within /lib, leaving /tag for interface and so on... uhm... not critical at all but I love to have libraries in /lib. For your consideration. Ciao
Hide
Mathieu Petit-Clair added a comment -

Thanks for the comments Eloy! Martin also commented on both things.. so, I changed the parameters (removed the array thingy) for the two "public" functions (tag_set and tag_set_add). It wouldn't be so much work to remove all the arrays, and actually, I was trying to come up for a good reason to use them after all while talking with Martin, but couldn't really... so I think I might modify it all, next week.

I had named the file taglib.php to make it obvious, while using moodle, which files depended on tag/lib.php. I renamed it lib.php, and separated a few "tag_print_*" functions in tag/locallib.php. I think that, as tags become more a part of "core moodle", the file should be moved to /lib/taglib.php, but this would require some work on making sure the right files get included .. and I won't attack this on a Friday night.

This is not perfect, though. The tag management interface needs a lot of work, among others...

Show
Mathieu Petit-Clair added a comment - Thanks for the comments Eloy! Martin also commented on both things.. so, I changed the parameters (removed the array thingy) for the two "public" functions (tag_set and tag_set_add). It wouldn't be so much work to remove all the arrays, and actually, I was trying to come up for a good reason to use them after all while talking with Martin, but couldn't really... so I think I might modify it all, next week. I had named the file taglib.php to make it obvious, while using moodle, which files depended on tag/lib.php. I renamed it lib.php, and separated a few "tag_print_*" functions in tag/locallib.php. I think that, as tags become more a part of "core moodle", the file should be moved to /lib/taglib.php, but this would require some work on making sure the right files get included .. and I won't attack this on a Friday night. This is not perfect, though. The tag management interface needs a lot of work, among others...
Hide
Eloy Lafuente (stronk7) added a comment -

Great Mathieu,

I really like the API itself, just were some "formal" details. You've done a good job reorganizing and fixing the tag system (I'm sure we'll use it intensively for 2.0 and so on). With arrays out from parameters next week... things will be really better!

Happy weekend anyway, really Friday isn't a good timing to break things, hehe!

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Great Mathieu, I really like the API itself, just were some "formal" details. You've done a good job reorganizing and fixing the tag system (I'm sure we'll use it intensively for 2.0 and so on). With arrays out from parameters next week... things will be really better! Happy weekend anyway, really Friday isn't a good timing to break things, hehe! Ciao
Hide
Petr Škoda (skodak) added a comment -

unfortunately I had to revert the commit, because it was missing a file tag/locallib.php which was preventing admin login and breaking tags code. tried to use the functions from the file above but encountered some other problems.

Show
Petr Škoda (skodak) added a comment - unfortunately I had to revert the commit, because it was missing a file tag/locallib.php which was preventing admin login and breaking tags code. tried to use the functions from the file above but encountered some other problems.
Hide
Petr Škoda (skodak) added a comment -

Mathieu please recommit it on Monday, I will have look at the code and we can polish the remaining details on Tuesday

Show
Petr Škoda (skodak) added a comment - Mathieu please recommit it on Monday, I will have look at the code and we can polish the remaining details on Tuesday

People

Dates

  • Created:
    Updated: