Skip to content
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

Add input data to processed messages #46

Open
wants to merge 11 commits into
base: 1.x
Choose a base branch
from

Conversation

SpartakusMd
Copy link
Contributor

@SpartakusMd SpartakusMd commented Nov 12, 2023

Fixes #17

  • Uses symfony/serializer to normalize the message from the envelope.
  • Stores the data into the input field
  • Didn't work on tests yet, until it's confirmed the approach is good
image

@kbond
Copy link
Member

kbond commented Nov 14, 2023

Thanks for the PR @SpartakusMd! I'm quite swamped at the moment so I'll give it a proper review (and start working on the other issues) when I have a chance.

Curious about what you said in the issue:

It gives the possibility to process the data later

Using symfony/serializer makes this difficult to do, right? If this is a valid use case, I wonder if we shouldn't use messenger's serializer. Then, when the user wants to re-run the message, we should just be able to unserialize and send to the bus. To keep the info available in the UI, we'd unserialize the data (using the messenger serializer), then normalize (using symfony/serializer). I hope that makes sense...?

<pre class="mb-0" style="white-space: pre; max-height: 340px; overflow: auto;">{{ value|json_encode(constant('JSON_PRETTY_PRINT')) }}</pre>
{% elseif value matches "\/\n/" %}
<pre class="mb-0" style="white-space: pre; max-height: 340px; overflow: auto;">{{ value }}</pre>
{% else %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test enums? Enum-Support could be added later (If not supported yet)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added enum support

@Chris53897
Copy link
Contributor

Thanks for the PR. Look already promising.
And of course a rerun option would be great.

@aborza aborza force-pushed the feature/add-input-data-to-processed-messages branch from 05ae1d4 to 5fd1238 Compare August 9, 2024 09:59
@aborza
Copy link

aborza commented Aug 12, 2024

The input appearance on details page:
Screenshot 2024-08-12 172731

@SpartakusMd SpartakusMd marked this pull request as ready for review August 12, 2024 16:38
@@ -13,8 +13,9 @@
],
"require": {
"php": ">=8.1",
"symfony/framework-bundle": "^6.4|^7.0",
"symfony/framework-bundle": "^6.4.1|^7.0",
Copy link

@aborza aborza Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To include the following fix symfony/symfony#52852 for symfony serializer

@SpartakusMd SpartakusMd requested a review from Chris53897 August 13, 2024 10:15
@SpartakusMd
Copy link
Contributor Author

@kbond what do you think about the implementation? We're using the fork now in production and improves a lot the debugging life.

@kbond
Copy link
Member

kbond commented Aug 22, 2024

Looks good, I'm going to be adding this bundle to an ap so I'll be able to test this out locally soon.

@SpartakusMd
Copy link
Contributor Author

@kbond I found an issue related to emails. If the email is sent via Messenger, it will also be captured and save to the database. The problem comes when the message contains attachments, they will also be serialized and saved to the database. I propose to clone the message before serializing and remove attachments from it. What do you think?

@SpartakusMd
Copy link
Contributor Author

In the end, I have added a new stamp DisableInputStoreStamp to disable saving the input.

@SpartakusMd
Copy link
Contributor Author

SpartakusMd commented Oct 16, 2024

Hello @kbond, did you have time to check the changes?

@kbond
Copy link
Member

kbond commented Nov 10, 2024

Hey @SpartakusMd!

Sorry for the delay - I haven't forgotten about this!

I'm pretty sure the next pre-release is going to require a migration because of #105 so feels like a good time to add this feature.

I think I'm going to add a global config to disable/enable the result storage by default, then add EnableResultStoreStamp/DisableResultStoreStamp (to use depending on your global config). Thinking we should do the same with this feature also (global config + EnableInputStoreStamp/DisableInputStoreStamp).

Also, for the Email attachment issue, could we detect when a field is binary (the case for attachments) and just replace with (binary)?

Thoughts?

@SpartakusMd
Copy link
Contributor Author

Hello @kbond, sounds good the config and stamps.

Regarding email attachment, I didn't find a way to eliminate binary fields from being saved in the DB.

@aborza aborza force-pushed the feature/add-input-data-to-processed-messages branch from d913392 to cc9cde9 Compare November 25, 2024 09:43
@SpartakusMd SpartakusMd force-pushed the feature/add-input-data-to-processed-messages branch from cc9cde9 to c95802a Compare November 25, 2024 13:59
@bendavies
Copy link

awesome feature!

@kbond kbond mentioned this pull request Dec 16, 2024
@SpartakusMd SpartakusMd force-pushed the feature/add-input-data-to-processed-messages branch from f3a1b6a to ac39231 Compare January 13, 2025 13:18
@SpartakusMd
Copy link
Contributor Author

@kbond could we move forward with this PR? I would like to add some extra functionality on top of it and avoid piling all the changes in a single PR, which would make the review harder.

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.

Include message data in the history
5 participants