Clean and fix phpunit tests - #9005
Merged
Merged
Conversation
AngelFQC
force-pushed
the
phpunit-tests
branch
2 times, most recently
from
September 3, 2026 20:29
bd6f30b to
c07c37c
Compare
UserRolesProcessor already compared the requested roles against the persisted ones, but it asked the entity itself whether the actor holds ROLE_GLOBAL_ADMIN. When a user edits their own account the actor and the target are the same managed entity, and denormalization has already added the role to it -- so the payload answered the authorization question it was supposed to be subject to, and any plain admin registered in the topmost URL could grant themselves the role. Reaching that branch already proves the persisted roles lacked it, which settles the self-grant case without consulting the mutated entity. Granting to another user is unchanged, and so is any edit to someone who already holds the role.
buildResourceLinkListFromContext() forces the link binding to the gated course, but nothing validated parentResourceNodeId: a teacher could post a document with their own cid and a parent node belonging to another course, and the node landed in that course's path, quota and subtree while its link stayed in their own. assertParentNodeIsInCurrentCourse() now requires the parent to be the current course node or live under it, at every entry point that sets a parent from the request. In the ZIP route the check runs before extraction, since saveZipContentsAsDocuments() writes the whole archive under that parent. The course context comes from CidReqHelper instead of reading the session by hand: CidReqListener is what populated it, and the ROLE_CURRENT_COURSE_* roles that gate these operations are computed from the same place, so a request that reaches the controller always has it. That makes the previous fallback to the query parameters unreachable, and it is gone with resolveCourseContext().
Declaring the name with backticks does not rename anything: the column stays "stored" and the schema is unchanged, so no migration is involved.
AngelFQC
force-pushed
the
phpunit-tests
branch
from
September 3, 2026 23:33
c07c37c to
ec87af1
Compare
Second pass over the PHPUnit suite. Most of these are the same count-delta conversion as the previous commit -- CForum, CLpItem, CShortcut, CSurvey, CQuiz, CQuizQuestion, CTool, CCourseDescription, CForumCategory, CLp, CoursesController and the three files left half-converted (TrackEExercise and CourseRepository had their baseline captured *after* createCourse(), so the delta was off by one). Fixing those unmasked assertions that had nothing to do with the demo courses and were simply outdated: - CLp and CQuiz assert the legacy overview/lp_controller URLs; getLink() returns the /resources/... routes now. - CQuiz::getMaxScore() returns float, so 0 is not 0.0. - CourseRepository::testCreate expects 24 course tools; ToolChain's list has 28. - testGetCourses expects a student to get a filtered course collection, but GET /api/courses is declared teacher/admin -- a student gets 403, and asserting that is the point. - PageRepository's slug: the slugger treats the apostrophe in "l'êtê" as a word separator, so the slug is l-ete, not lete. And createDefaultPages() creates 9 categories besides the admin blocks, not 8 (introduction was added). - LanguageFixtures enables every language it ships, so "11 available" is dead. The test now creates an unavailable language and asserts it is the one left out, which is what getAllAvailable() is for. - SettingsCurrent asserts accessUrlLocked is 1 right after setting it to 0. - CShortcut asserts the shortcut dies with its course. It does not: Course::$resourceNode only cascades 'persist', and CourseRepository::delete() resolves child nodes through getResourceFromResourceNode(), which queries the Course entity and so never matches a child resource. Deleting a course removes the course row alone and orphans its resource_node subtree. Documented in place next to the Fixme that already covers the sibling case. CSurvey::testCreateWithQuestions still errors, on the course deletion itself, for an unrelated reason -- see PHPUNIT-CI-HANDOFF.md.
Deleting a course could die with ORMInvalidArgumentException: Binding entities to query parameters only allowed for entities that have an identifier. Class "Course" does not have an identifier. CTool declares the course as a Gedmo\SortableGroup, so every CTool removal makes the SortableListener queue a relocation of the group's remaining positions and run it from postRemove -- after the DELETE, when Doctrine has already nulled the Course's identifier. Course::$tools cascaded 'remove' with orphanRemoval, which put the tools and their group in the same flush, so whether it blew up came down to the commit order Doctrine picked: a bare course survived, and one referenced by other entities (a survey invitation, in the test that caught this) did not. The ORM cascade was redundant to begin with: c_tool.c_id is ON DELETE CASCADE, so the rows go with the course either way, and the whole resource_node subtree is already left to the database. Dropping it keeps the deletion out of the UnitOfWork, so Gedmo never sees it. orphanRemoval goes with it -- nothing in src/ removes a CTool by detaching it from the collection. Fixes CSurveyRepositoryTest::testCreateWithQuestions.
The parameter was still called $active, from the days when it filtered c_quiz.active. Commit eb758d7 removed that column and replaced the filter with a hardcoded links.visibility = 2, which left the argument accepted and discarded: passing 0 returned the published exercises instead of none.
- Illustration counts the global illustration total, so it takes the same before/after delta as the rest of the suite. - AccessUrl asserts a new portal is the root of its own nested set. It is not: it joins the installed portal's tree as a leaf, so lft/rgt/lvl are 2/3/1. - GradeBookCategory persists a GradebookCertificate without a creator. That is a resource, and with no authenticated user in the test ResourceListener has nowhere to take one from, so it throws UserNotFoundException.
testImportDocumentMetaResolvesFileFolderPathCollisionFromFilesXml wrote its fixture into files/aa/ without creating it first, unlike the sibling tests that mkdir their own hash directory.
LearningPathRuntimeProgressManager takes the repository in its constructor but only ever calls find() and findBy() on it, and the progress calculations under test never touch it. PHPUnit cannot double a final class, so six unit tests of pure SCORM/Rise progress arithmetic errored out before running. The keyword was added in bulk when the file was created, not as a decision about this class, and most repositories in src/ are not final.
MailHelper::send() ended in `Address("")`, which throws
`Email "" does not comply with addr-spec`, so a message POST answered 500. Neither
mail.mailer_from_email nor admin.administrator_email has a value on a database
that never went through the installer, which is every CI database.
A mail with no From cannot be delivered, so send() now returns false the way it
already does for an invalid recipient, and logs why. The reply-to had the same
hole: Assert\Email accepts the empty string -- it is not NotBlank -- so the guard
above it passed and the Address constructor rejected the value anyway. Both now
go through api_valid_email(), which send() already used for the recipient.
getPlatformFromAddress() builds an Address the same way and has the same hole. It
has eight callers, all commands and mailers that would each need an error branch,
so it is out of scope here and recorded in PHPUNIT-CI-HANDOFF.md.
MessageRepositoryTest sets a sender address for itself, and its "mark as read"
request switches from PUT to PATCH: receiver is not writable, and a PUT replaces
the whole representation, so it arrived null and the request 422'd. The Vue
service already overrides update() to PATCH for the same reason.
Eight CourseInvitationTokenService tests errored with "Cannot retrieve the container from a non-booted kernel", thrown from api_get_path() while building an invitation URL. Booting a kernel does not dispatch kernel.request, so LegacyListener -- the only thing that calls Container::setContainer() in a web request -- never runs in a service-level test. The static therefore keeps whatever container an earlier test left there, and that kernel has since been shut down, so the first legacy helper that reads it dies. Which test blows up depends on execution order. Refreshing it in setUp() puts legacy code in the same state a request gives it.
ToolChainTest::testCreateTools deleted every BranchSync, AccessUrl and ResourceType to prove createTools() reseeds them. On a seeded database the demo courses' resource nodes reference those types, so the flush died with ORMInvalidArgumentException; and the reseed path is covered for real by the Playwright install scenario, which runs the web installer. What is left is the idempotency check, which is the part of the method that was actually about the tool chain. The tail that built a ResourceType and a Tool to assert they got an id went with it. ResourceRepositoryTest called getResourceNodeFileStream() on AccessUrlRepository. That method lives on ResourceNodeRepository and takes two arguments; ResourceRepository only wraps the content getters, so the call was a BadMethodCallException. The assertion two lines above already proves the stored file is readable.
… the API
ResourceNode::$creator sat in the resource_node:write and document:write groups,
so it was mass-assignable: a teacher who passes is_granted('EDIT', object) could
PATCH a node and hand its ownership to any other user. Confirmed against the
regression test, which read the creator back as the attacker's id.
The creator is assigned by ResourceListener, from the resource or from the
authenticated user, so nothing legitimate writes it -- no caller in src/ or
assets/ sends the field. Read-only group only, which is what CLAUDE.md already
prescribes for an owner relation.
…s two callers The action gained a CidReqHelper parameter ahead of AiDisclosureHelper, and neither caller was updated, so both passed the disclosure helper into the CidReqHelper slot: TypeError: CreateDocumentFileAction::__invoke(): Argument #8 ($cidReqHelper) must be of type CidReqHelper, AiDisclosureHelper given Every MCP course-document creation died on it -- surfaced as "The course document could not be created because of an unexpected server error" -- and so did the AI-generated media storage path, which invokes the action the same way. This makes the calls type-correct. It does not make them work end to end: CidReqHelper resolves the context from the session, and neither of these callers runs inside an HTTP request, so the link comes back bound to no course. See PHPUNIT-CI-HANDOFF.md.
…nds to
Fixes the second half of the MCP document break. The TypeError is gone, but the
documents still came out with no course link at all, and this is why:
CreateCourseDocumentTool and GeneratedMediaStorageService build a synthetic
request carrying the course -- Request::create('/api/documents?cid='.$courseId) --
and hand it to the action. CidReqHelper does not read the request it is given: it
goes to requestStack->getCurrentRequest()->getSession(). The synthetic request is
never pushed onto the stack and never passes through CidReqListener, so that cid
was invisible, getCourseEntity() returned null, and
buildResourceLinkListFromContext() dropped the cid on its `if ($cid > 0)` guard.
The node ended up with zero resource_link rows, which means the document does not
show in the course, does not count against the quota, and
titleExistsInParentFolder() -- which requires a base-course link -- never sees it,
so MCP-created documents never got their duplicate titles disambiguated.
AngelFQC
force-pushed
the
phpunit-tests
branch
from
September 3, 2026 23:58
ec87af1 to
c40cc04
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.