-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: ViewerController
#379
Fix: ViewerController
#379
Conversation
Can you please approve the workflow run? Thanks |
@OskarStark Thanks for the PR. |
Any clue why the tests are failing? |
@OskarStark pipeline is merging this branch against |
This is correct, I target |
@DamienHarper can you please approve the workflow again? I extends the AbstractController again, lets see if that helps, thanks |
I don't know why
is failing 🤷♂️ |
Any update on this PR? Im eagerly waiting for this to be merged and released since we have customer projects using this bundle 😇 |
I really don't know why this test is failing |
Probably you are still extending from SF AbstractController and therefore renderView return type does not match
|
In Github Actions log I see this:
+1 for removing |
I tried in a previous commit but was still not working. Will try to remove it tomorrow again |
Removed again, lets see ... @DamienHarper is it really needed, to reapprove the workflow run all the time? |
Tests are still failing @DamienHarper, any idea? |
It looks like this is causing the issue: public function showTransactionAction(Reader $reader, string $hash): Response
{
$audits = $reader->getAuditsByTransactionHash($hash); |
@OskarStark When looking at the GA CI logs, it looks like it checkout hash |
Can you approve again, it should checkout the PR code itself and not the target ref IMHO |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 5.x #379 +/- ##
=======================================
Coverage 93.14% 93.14%
=======================================
Files 15 15
Lines 350 350
=======================================
Hits 326 326
Misses 24 24 ☔ View full report in Codecov by Sentry. |
@OskarStark hurray! you made it ;) |
* Fix: `ViewerController` * - * - * - * Fixed remaining calls to AbstractController methods. * - * - * - * - * PHP-CS-Fixer --------- Co-authored-by: Damien Harper <[email protected]>
Nice, can you tag a release? |
Done! (5.2.5) |
Thank you @OskarStark and @DamienHarper ! While I love this implementation (decoupling the controller from the framework), the only downside I can image is: the |
@mvhirsch you're welcome ;) |
* Updated CI * Fixed middlewares override (#341) * Revert unwanted PHP >= 8.0 changes * Updated branch alias * Reverted and adjusted middleware test * Adjusted middleware registration priority (fixed failing tests) * Fixed unwanted deps changes * Fixed failing test * #345 Deprecation in RoleChecker and UserProvider (#350) * #345 Deprecation in RoleChecker and UserProvider * remove line --------- Co-authored-by: a.dmitryuk <[email protected]> * Fixed CI (PHPStan) * Support doctrine/annotations:2.0 (#351) * remove doctrine/annotations from composer.json * doctrine/annotations 2.0 --------- Co-authored-by: a.dmitryuk <[email protected]> * Fixed compatibility with Symfony 4.4 * PHP-CS-Fixer * Fixed CI (#367) * PHP-CS-Fixer * Fix: `ViewerController` (#379) * Fix: `ViewerController` * - * - * - * Fixed remaining calls to AbstractController methods. * - * - * - * - * PHP-CS-Fixer --------- Co-authored-by: Damien Harper <[email protected]> * CI updates * Make `ViewerController` final (#385) Co-authored-by: Damien Harper <[email protected]> * Test more PHP versions (#389) Co-authored-by: Damien Harper <[email protected]> * Use `actions/checkout@v4` (#387) Co-authored-by: Damien Harper <[email protected]> * PHP-CS-Fixer * Update deps + PHP-CS-Fixer * PHP-CS-Fixer (3.48.0) * 6.x Signed-off-by: Oleg Andreyev <[email protected]> * reverted ci-5.x.yml Signed-off-by: Oleg Andreyev <[email protected]> * reverted ci-5.x.yml Signed-off-by: Oleg Andreyev <[email protected]> * clean composer.json Signed-off-by: Oleg Andreyev <[email protected]> * reverted ci-4.x.yml Signed-off-by: Oleg Andreyev <[email protected]> * damienharper/auditor dev-master Signed-off-by: Oleg Andreyev <[email protected]> * allow ^3.1 orm Signed-off-by: Oleg Andreyev <[email protected]> --------- Signed-off-by: Oleg Andreyev <[email protected]> Co-authored-by: Damien Harper <[email protected]> Co-authored-by: Alexander Dmitryuk <[email protected]> Co-authored-by: a.dmitryuk <[email protected]> Co-authored-by: Oskar Stark <[email protected]>
After upgrading to Symfony 6.4, the ViewerController stopped working while throwing the following exception:
Upon investigation with @mvhirsch, we discovered that the service_container injected into the controller lacked the twig service. However, this shouldn't have been a problem since twig was injected directly via arguments. The root cause turned out to be the use of
AbstractController::render()
method, which attempts to access twig from the service container, instead of the intendedViewerController::renderView()
.Additionally, I've realized that there is no necessity to inject the service_container or extend the
AbstractController
. We’re currently running tests to see if this resolves the issue.Thank you for your assistance and insights