Symfony7 #1

CunningFatalist · 2024-04-08T10:36:11Z

Ticket

@paroe

I will NOT merge this PR, it's just here to make your live easier.

This should fix all deprecations and composer issues.

CunningFatalist · 2024-04-10T04:25:45Z

@paroe Found some more issues with PhpStorm. They are fixed now and this is ready for review.

paroe

I found some issues, mainly in the serialization of the StackTrace / Exception. I'll try to fix this

paroe · 2024-05-03T16:15:05Z

Entity/Type/SafeObjectType.php

+use Doctrine\DBAL\Types\JsonType;

-class SafeObjectType extends ObjectType
+class SafeObjectType extends JsonType


This causes some trouble. We currently have data in the database and that is stored (in Application:87) by serializing a FlattenException. That means that jobs without an error always have N; as the value, and jobs with an error a mega long hex value that looks something like 0x4F3A35373A2253796D666F6E795C436F6D706F6....
This of course can't be handled by the JsonType, so the worker do not start if there are existing jobs.

So, we are dealing with two problems here:

the value in the database has to become a JSON for existing jobs

the value in the database has to become a JSON for new jobs

And a third one:
3. This has to be done ideally in a backwards compatible way so that we can migrate back if we need to and ideally have the old and the new workers running at the same time (probably not possible)

The first problem can probably be solved by a migration. But that is not backards-compatible then. So, maybe we have to consider to add a new column.

I tried json_encode for the FlattenException, but unfortunately that just returns an empty JSON object. I was not able yet to find another way to get a proper JSON from nested exceptions. I think that we maybe have to consider just saving the stacktrace as string, but then we lose some valuable information, like the exception message.

I'll try to create a JsonSerializable FlattenException; that should solve that problem. And I am thinking of either rewriting the JobSafeObject to handle both - the old and the new - style in the database. But then we still have to perform a double deployment. I think it might be easier to write a migration that rewrites the stuff in the database, but I fear that this is can't be reversed. So the better solution is probably to introduce a new column and just set the existing one always to N; until we can finally remove / rename it.

As far as I remember, that's a change that I had to make. I didn't stumble across those implications during my testing. So my current status is that you don't need help with this (anymore)?

yes exactly; I have pushed the updated code with all my fixes that were required (see also my other comments)

I didn't stumble across those implications during my testing.

Probably because you didn't have old jobs in your database that were already executed. The stacktrace is only written when a job has been started and not when the job is still pending. So, this is an edge case that our tests can not really cover and I also only discovered this when I tried to activate the workers on Staging

paroe · 2024-05-03T16:22:14Z

Repository/JobRepository.php

+ * @method Job[] findAll()
+ * @method Job[] findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)
+ */
+class JobRepository extends ServiceEntityRepository


Did you have to create this repository? In Indiana I am getting an error about a missing service when I try to open an job in the admin view. I'll try to remove this repository, maybe that also works (as the JobManager does all the repository stuff already)

It was automatically created and I thought it wouldn't hurt. But then again, it does. Do you want me to remove it?

I am not sure, why exactly this is an issue. I assume that we might have to rewrite the bundle's DI to fix this if we wanted to use the explicit repository. But as it is not needed I just removed it.

paroe · 2024-05-13T16:06:13Z

Command/RunCommand.php

    public function __construct(ManagerRegistry $managerRegistry, JobManager $jobManager, EventDispatcherInterface $dispatcher, array $queueOptionsDefault, array $queueOptions)
    {
-        parent::__construct();
+        parent::__construct('jms-job-queue:run');


I had to move the name to the constructor because the constructor calls configure at the very end. And if you then call setName after the constructor already ran you can't extend commands anymore. This normally is not a problem, but Indiana extends the RunCommand (app:jms:run) because we have to add an additional parameter ("hostname"); and this setName made our command unavailable as the original one took precendence.
And there is also some extra logic regarding aliases in the constructor that is not executed in this case, but we don't need that

Oh damn, good point.

paroe · 2024-05-13T16:09:50Z

Console/Application.php

                'memoryUsage' => memory_get_peak_usage(),
                'memoryUsageReal' => memory_get_peak_usage(true),
-                'trace' => serialize($ex ? FlattenException::create($ex) : null),
+                'trace' => $trace,


In the past the data was a serialized PHP object; but now it has to be valid JSON.
I also tried to add a new column for this (so that old runners that are still running can keep that column and new runners can use the new column); but this caused a lot of trouble of invalid database schemas (because of the unknown columns) and also caused trouble because of the SafeObject type; so in the end I decided that we have to make sure that either old or new runners are running exclusively and that we have to migrate the database manually if we want to switch between the runners. That is quite ugly, but the only thing that did not require implementing a custom database adapter to circumvent the validation errors

I don't even think it's that ugly; it's an okay solution. Nice fix

paroe · 2024-05-13T16:17:02Z

@CunningFatalist I committed my changes to fix the issues that I found, see this commit. I guess I maybe should have created a new PR to make the review easier, but in the end only very few things have changed. I have just added some types to the Job entity to get rid of some of the deprecation messages

CunningFatalist · 2024-05-14T08:07:57Z

Console/Application.php


+        try {
+            $trace = json_encode($ex ? FlattenException::create($ex)->toArray() : null, JSON_THROW_ON_ERROR);
+        } catch (\JsonException) {


You could use this, but it's not really important.

CunningFatalist · 2024-05-14T08:08:51Z

@paroe

Thanks for fixing this. The commit you linked looks good.

most of the changes have been ported from https://github.com/schmittjoh/JMSJobQueueBundle/tree/php81

CunningFatalist and others added 2 commits April 2, 2024 12:45

Update composer.json

1646d5e

IND-512 fix deprecations

a612c22

CunningFatalist requested a review from paroe April 8, 2024 10:36

CunningFatalist self-assigned this Apr 8, 2024

IND-512 fix route deprecations

29f7c47

CunningFatalist added 7 commits April 10, 2024 13:20

NOKEY fix interface

9ba0298

NOKEY more fixes

fc86ae8

IND-515 tree builder return type

92b2235

IND-515 mapped super class

ecf7e53

IND-515 attributes

39e25a9

IND-515 attr

b310465

Fix type

dbb8d3e

paroe requested changes May 3, 2024

View reviewed changes

IND-515 fix compatibility issues

46346a9

paroe force-pushed the symfony7 branch from 67a5448 to 46346a9 Compare May 13, 2024 16:02

paroe reviewed May 13, 2024

View reviewed changes

CunningFatalist commented May 14, 2024

View reviewed changes

paroe added 3 commits May 17, 2024 17:24

IND-515 fix type annotation

bc0dbd9

IND-515 fix some more issues

5ad649e

most of the changes have been ported from https://github.com/schmittjoh/JMSJobQueueBundle/tree/php81

IND-515 document expected args structure

fb695f5

Symfony7 #1

Are you sure you want to change the base?

Symfony7 #1

Uh oh!

Conversation

CunningFatalist commented Apr 8, 2024

Uh oh!

CunningFatalist commented Apr 10, 2024

Uh oh!

paroe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paroe commented May 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CunningFatalist commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants