Skip to content

Conversation

@Coussecousse
Copy link
Contributor

Original pull request : #6896

Hi! Sorry this is taking sooo long!

I looked at the code and your reviews @ywarnier, and I did the following :

  • Removed the ECS fixe typo commit
  • Used the course entity to get the sessions and retrieve the group
    I didn’t manage to use CidReqHelper because the request does get the courseId and the course entity, but not sid and gid.

Dump of the request -> you can see cid and course :
Capture d’écran du 2025-12-16 14-49-59
Differents dumps :

  • Line 35 : The course
  • Line 2471 and 37 : api_get_session_id()
  • Line 43: Just again the request from CidReqHelper
  • Line 54 : sessionId using $this->cidReqHelper->getSessionId();
Capture d’écran du 2025-12-16 14-50-20

So if the session is null, then I can't know what size is used (and the calcul crash)...

However, using the course entity still allows the feature to work properly.
Capture d’écran du 2025-12-16 15-34-19

I’ll let you review this hopping there's not much to correct :D !

@ywarnier
Copy link
Member

ywarnier commented Dec 18, 2025

Hi @Coussecousse
No problem about the timing, we all have many things to do and this is not a priority for right now (but we have to close it before the stable).

I notice in CDocumentRepository.php you're still using a path comparison.

We did a change recently which affects considerably the way we work with documents, in that the c_document table (mapped by the CDocument entity) is not the main way to list documents anymore. Rather, the resource_link table has been modified to include a "parent_id", which helps reflect the hierarchical structure of the documents in any course context (because we intend on making documents shareable between courses and we still need them to maintain a local-context hierarchy, which we couldn't do with c_document anymore).

This means that the right way to get the "root" (the equivalent of the "/" path) is not to check for "/" in the path anymore, but rather to check for the "eldest parent" in the resource_link hierarchy of nodes, or (to be more efficient) the direct descendant of the document "tool" (which is also a node of the course).

I'm not entirely knowledgeable on the way to manage this in Chamilo right now (this is a recent change), but I'm adding @christianbeeznest here to review that part of your PR. The rest seems fairly reasonable and your explanation about the sid is satisfying to me (maybe Christian will add something to this, but I think it's fine).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants