Refactor 'comment' and 'variables' GiftcardTransaction base field definitions

The 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_id and reference_type so 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:

'comment' => $this->t('Used on order with id @order_id.', ['@order_id' => $order->id()]),

'comment' => $this->t('Bought @product.' ['@product' => $purchased_entity->label()]),

Hi and thanks for the feedback. I'm also unsure if comment is 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 variables should be removed. A static string would be fine as comment and can / should be generated at the time of creation.

Updated the issue summary and title.

If 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.

Please 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.

Update 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.

@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.

Just 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.

Yeah 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.