Closed (fixed)
Project:
Commerce Gift Card
Version:
2.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
11 Apr 2025 at 13:38 UTC
Updated:
11 Jun 2026 at 07:30 UTC
Jump to comment: Most recent
Problem/Motivation
The 'variables' GiftcardTransaction base field definitions seem very out of place.
'comment' is a string containing replacement characters, like '@order_id' and 'variables' holds the "real" values of these replacement characters (e.g. "123" for order ID 123).
Both fields can not be filled by the user through the GiftcardTransaction form, and they are only used through the "OrderEventSubscriber", which fills them with either:
'comment' => 'Used on order with id @order_id.',
'variables' => ['@order_id' => $order->id()],
or
'comment' => 'Bought @product.',
'variables' => ['@product' => $purchased_entity->label()],
Steps to reproduce
Proposed resolution
Remove the "variables" base field entirely and discuss whether to keep 'comment' or not.
Let the user add a comment via Form if we want to keep it.
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork commerce_giftcard-3518763
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
rhovlandThe comments field is definitely useful. Think of it like how commerce_log works. It keeps a list of transactions with details about what happened. The log can be what order a gift card was used on. It can be a gift card was purchased.
What is missing is when a transaction is added by an admin using the "add transaction" button, the comment is blank. That's not good. The admin should be able to enter a comment about the transaction they were creating.
The transaction entity actually already stores the order id associated with the transaction in
reference_idandreference_typeso an additional field would be redundant.I think we can safely drop the variables field and keep what is already being recorded in the comments but doing it the correct way like this:
Obviously the code that consumes the comment field will have to be adjusted because it currently combines the comment with the variables such as in getComment().
Comment #3
anybodyHi and thanks for the feedback. I'm also unsure if
commentis really essential. Still it's not worth a long discussion and I'm fine to keep it, if it helps someone. It's much personal preference and learnings, I also like ways to keep (admin) notes for entities... still it doesn't have to be within the entity itself.Despite that, I totally agree that
variablesshould be removed. A static string would be fine as comment and can / should be generated at the time of creation.Comment #4
grevil commentedUpdated the issue summary and title.
Comment #5
rhovlandIf comments on transactions could happen at a separate time from when the transaction was being created I could see it being apart from the entity. But it is part of the creation of the transaction and it makes sense there. Think of it like a comment on a payment (not a payment method) where it's a snapshot in time of something that happened, with additional relevant information attached to it.
If we added comments for gift cards then it would be inappropriate to put it on the giftcard entity.
I'm going to try to tackle this issue and get some code put together for review.
Comment #6
rhovlandPlease verify if I have done the translation part in the event subscriber correctly. I used commerce core as a guide.
Comment now shows up on the admin form.
Going to write an update hook next.
Comment #8
rhovlandUpdate hook written.
The update fills in the variables and resaves the comment. Once that is done it removes the variables field from the database. I have to read the variables directly from the database because the field no longer exists in the entity.
I tested this update hook on a copy of our production database and examined the table contents before and after and all comments were correctly updated.
Comment #9
grevil commented@rhovland thanks for your work on this!
The translation logic is not perfect yet. The "comment" base field is exposed to the user UI, so we can't simply pass a t string as its parameter. The user won't ever be able to do that via UI.
Theoretically, we need to set the comment base field definition translatable and then pass "normal" markup to the field, when using it programmatically.
But I am a bit unsure about this, as we never want to translate the entity itself, but only want to translate the comment. So maybe the current code is the way to do it. But in this case we can't expose the comment field in the form. What do you think @anybody?
EDIT: Note, that I haven't checked the update hook yet.
Comment #10
grevil commentedJust talked with @anybody internally. He thinks, that comments should generally not get translated in this context (when set by user). Although I feel like this is a special case, where the comment gets hardcoded as well.
So I guess we should use FormattableMarkup instead of $this->t(). Otherwise, this works great! Just looked over the update hook code and tested it. Works great and adjusts the DB and config accordingly. Thanks!
Back to NW for final change.
Comment #11
anybodyYeah in general this is just my 2 cents. We should not be too strict with this, it won't harm too much if it's not perfect yet.
Comment #12
rhovlandRebased and changed to using FormattableMarkup
Comment #13
anybodyThank you @rhovland! If you agree I think this is ready for RTBC! :)
Comment #14
rhovlandI reviewed the code again.
Ensured usage of FormattableMarkup followed the security recommendations in that class' comments.
Tested on a copy of a production site.
Merged.
Comment #17
grevil commentedNice! I'll release alpha 3.