🇬🇧United Kingdom @jonathanshaw

Great

I kept meaning to finish what I started here
https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →

jonathanshaw → created an issue.

I think this is ready now.

I can see the argument that the previous is more precise. I don't think it matters either way.

jonathanshaw → created an issue.

jonathanshaw → changed the visibility of the branch 3520997- to hidden.

jonathanshaw → changed the visibility of the branch 11.x to hidden.

jonathanshaw → changed the visibility of the branch 3520997-defaultlazyplugincollection-unnecessarily-instantiates to hidden.

jonathanshaw → changed the visibility of the branch 3520997-2 to hidden.

Anyone interested in this issue likely also interested in 📌 DefaultLazyPluginCollection unnecessarily instantiates plugins when sorting collection Active .

jonathanshaw → created an issue.

@mxh can you RTBC this?

What I'm not sure about is whether changing this will reintroduce the bug found in #2431379: LazyPluginCollection should not implement \Iterator → .

Agreed. Php only requires \Traversable to be returned by \Iterable::getIterator, it doesn't need to be \ArrayIterator.

It looks like this was given \ArrayIterator in 📌 Fix root namespace classes DebugClassLoader forward compatibility warnings Needs work , as part of a project to clean up a whole set of unrelated type hints. There was no discussion of the specific needs of plugin lazy loading.

In fact, we should update the provided implementation to use a generator. It's clearly in line with the whole purpose of LazyPluginCollection to do so.

jonathanshaw → made their first commit to this issue’s fork.

First of all, I appreciate the strategy. I think splitting "product" and a hardcore core is much better than trying to compromise in the middle between the two.

I also appreciate the strong mentions of third party integrations. We don't do much to help live data integrations and I imagine it's a common enterprise need.

But I'd also like to leave some more challenging feedback.

I think this would have been a visionary strategy 10 years ago, and a good strategy 5 years ago. Today, it reads (to borrow a trope) like a general's memo on how to win the last war. It's not wrong, but it contains a hidden assumption and
omits a critical future issue.

What assumption? That the crucial user of Drupal's cores APIs is a human. What issue? The importance of making Drupal the platform of choice for AI assisted Drupal product owners, architects and developers.

We face a world as early as 2026 of agentic coding tools where decision makers expect it to be straightforward for well-specified feature requests for their site to be implemented near instantaneously, and have little patience when that fails.

What does this mean for Drupal?

For Drupal CMS it means things like:
1. Finding a way to make configuration maximally comprehensible for AI so it can be generated and modified reliably.
2. Adding modules like ECA or twig formatters to massively enhance the range of custom features that can be delivered safely by configuration alone.
3. Creating an AI sitebuilder module that can turn natural language requests into reviewable configuration changes.

For Drupal core it means things like:
1. Constantly studying what parts of core are intrinsically difficult for AI to work with reliably and fixing them. Render arrays might be a candidate.
2. Remove footguns that make it easy for AI to generate code that works with simple manual testing but contain subtle security, performance and unexpected input bugs.
3. Treating dev tooling like a mission critical priority not a matter of personal taste. Providing MCP tools that help AI understand the idiosyncrasies of a Drupal project. The existing API module is an example to build on. An MCP to expose the Drupal data model would be another possibility.
4. Understanding that bug-fixing AI generated code becomes a crucial focus of attention. When features become easy to create, frustration is high when bugs are discovered later. How can we make it easier to specify a bug and have AI trace it?

You may well want to reply at this point:
Yes, that's all very well, but those are implementation issues not strategy at the highest level that we're concerned with.

You may even be right to say that. I'm not sure the issues I raise negate the strategy. But they do point to a potential shift in competitive environment and the mentality of decision makers, that should be considered carefully when evaluating strategies.

Let's prepare for the next war, not the last one.

Thanks for #3515814--16059987 🐛 PaymentElement assumes next step should be 'complete' Active , it's very helpful. From that:

It is possible that the customer completed the payment with Stripe, but the onReturn is NOT invoked. (This can happen when the customer's network connection drops, battery dies, etc...) In this case, the webhook necessarily handles creating the payment and placing the order.

This seems to be the won't fix answer to this present issue:
(1) the onReturn() methods needs to use the same decison making logic about the order status in response to a successful stripe payment that the webhook uses when it handles a successful payment where the customer has dissappeared from checkout after Stripe takes the payment. It'd be confusing if an order was sometimes placed and sometimes not after payment.
(2) Most sites want the order to be placed after payment is received. It's the 80% use case. Sites that want different can customise the payment gateway's placeOrder() method which is used by both the webhook and onReturn and exists as an seperate method for the very purpose of facilitating customisation of this order handling.

What I'm not clear about is the matter raised in the IS:

According to the interface \Drupal\commerce_payment\Plugin\Commerce\PaymentGateway\OffsitePaymentGatewayInterface:

*OnReturn: Processes the "return" request.
*
* This method should only be concerned with creating/completing payments,
* the parent order does not need to be touched. The order state is updated
* automatically when the order is paid in full, or manually by the
* merchant (via the admin UI).

*

"The order state is updated automatically when the order is paid in full". This appears to refer to the work of
\Drupal\commerce_payment\EventSubscriber\OrderPaidSubscriber::onPaid() which does indeed do exactly that, for any order.

So I think it might be sufficient for the placeOrder() method to simply save the order, triggering OrderPaidSubscriber::onPaid().

Dispatching CheckoutEvents::COMPLETION) from placeOrder() is a bit of a weird one. Is the checkout really complete if the customer abandons it after payment? There's an argument it should only be called from the webhook, not from onReturn (because in onReturn the customer hasn't actually completed checkout necessarily); but there's also an argument for consistency between the two situations. I can see it both ways.

I think we can't get away without depending on IEF at least some of the time. When we have existing evidence and want to allow staff to add more it's the obvious UX.

IEF is generally fine if you don't need to nest one IEF inside another. It's nesting that is especially flaky. And in our case we don't need to do that, so we should be fine.

Given the budget constraints, I think we should go with IEF as the bare minimum working solution to get us to a MVP solution for the module as a whole. Once we have that MVP, we can revaluate what if any problems we have budget to fix.

Nor does it have a label, and I don't see any sensible way to create one.

AFAIK a lot of stuff breaks without a label. I'd use the bundle name as the label for now. The paragraphs module has prior art on autogenerating a text summary from arbitrary fields, but that's unecessary chrome for now.

Staff look up the Donor, or create one if it doesn't yet exist.
From the Donor page, select an evidence type using a drop-down, then click the "Add evidence" button.
This brings up a page for creating evidence, with the donor ID included in the URL query string. The page includes an "options select" field to choose what action the evidence is for: declare, cancel, change of address, or misc correspondence. When staff click "Create", they are redirected to the correct next page, either editing the Donor, or creating a declaration/cancellation entity.
The new page URL includes the evidence ID in the URL query string and also the Donor ID for the cases where we create the entity. The corresponding fields are pre-filled and hidden/disabled in the form.

Youre quite right that using a 2 page form process is a viable alternative approach.

The obvious approach for a declaration is like this:
1. When staff create a declaration, the declaration create form redirect is set to the evidence bundle add page (where they select the bundle)
2. They select the evidence bundle
3. This takes them to the evidence add form. The declaration id from step #1 is persisted through the URl to this point, an the evidence is added to the declaration on submission.

The problem with this approach is that the declaration exists in a weird draft state at first if the evidence is not submitted. In the approach above it doesn't even know if it's oral or not at first. We can get round this by moving the bundle select into the declaration create form, but it doesn't avoid the need for a draft declaration state of some kind, and likely some cleanup of drafts that don't get finished.

For cancellation, change of address etc, your approach had staff choose the evidence bundle first and the action second; I think t's important to reverse that. Staff should be first choosing an action that expresses their goal "cancel", "Change address", etc., before they have to think about evidence.

Overall I suggest seeing how far you get with IEF OOTB. Customising IEF is possible - there are sane ways of doing it, but it can also get hairy fast so I suggest discussing with me before entering that rabbit hole too far.

I already use IEF in my project in myriad ways (!nested!) so the dependency on it here is not scary for me at all.

end_date (someone could ask not to claim Gift Aid on their £25000 donation from savings as they didn't pay enough tax, but keep claiming on the £50 direct debit; one way we could handle this is a cancellation to exclude a date range)

Very interesting. An obscure use case I know I will have, but have avoided mentioning to keep things simple as it could be handled manually, is ceilings on what can be claimed. e.g. in the year 19-20 don't claim gift aid worth more than £xx as that was all the tax I paid.

Your cancellation entities seem well suited to being extended for this kind of use.

I'm happy with the idea of donor revisions + cancellation entities. Seems quite a strong solution to me.

As you say, cancellation has quite fiddly requirements unique to it.

Happy to leave dynamic contexts and donor revisions aside for now if they're not the approach you're most comfortable with.

The case I'm most concerned about if we don't have donor revisions and use change of address records instead is if we (by default, or a site chooses) to update the donor address to match the default commerce address in the address book. Then it's not as simple as custom code simply updating the address on the donor entity, there's also the change of address record to create.

We could of course have the door entity autocreate a change of address record as a kind of pseudo-revision. But maybe the right response is to say: it's right to give custom code the problem of creating a change of address record, because they need to consider audit and provide an explanation of why they made the change they made. But I'm still left wondering: isn't revision log the API Drupal already gives us for this ...?

These provided bundles are to be helpful, customisation is expected, so let's not overinvest in them. Given that, yes to everything except:

Block staff access to create/edit declarations with evidence type "web".

Better:
- block staff access to create/edit evidence with type 'web'
- block staff access to edit a declaration whose owner is the declarant

I suspect this is a duplicate of 🐛 \Drupal\plugin\PluginType\PluginTypeManager::getPluginTypes() does not use file cache correctly Needs review

jonathanshaw → created an issue.

See https://git.drupalcode.org/project/entity/-/tree/8.x-1.x/src/BundlePlugin

And

https://git.drupalcode.org/project/entity/-/tree/8.x-1.x/tests/modules/e...

Commerce_payment uses this for bundles of the payment gateway or payment method entity type I think.

But also when I said about putting logic in postSave() on the bundle, I was mixing it up with the bundle classes of https://www.drupal.org/node/3191609 → .

Unfortunately a class can't be both a bundle plugin and a bundle class as they need a different parent class.

But we could have an onCancel() method on a GiftAidRecordBundlePluginInterface and call that from the GiftAidRecord::postSave() entity method.

I'm thinking MCP is the way to go here. A ddev extension could provide a set of MCP tools that help working with Drupal codebase but which would work in any AI dev tool, Claude code, roo code, cline, cursor, etc. They all support MCP.

I guess that stripe might not know that the card is disposable

They must know, or otherwise why do they prevent it being attached to a customer?

(1) If you were able to provide a complete [redacted] payment intent object JSON here that might be useful.

(2) Also, it's be good to know what the exception was that was thrown, what it's message was, and what stripes cloud logs show.

(3) Stripe's test docs say that pm_card_visa_chargeDeclined can't be attached to a customer. So you could try creating a test customer and attaching that and seeing what error you get.

💬 Anonymous Checkout Denied When Order is Programmatic Fixed is an old discussion but possibly relevant.

jonathanshaw → created an issue.

I think this is ready now, but I've not tested it properly and I'm reluctant to put more work into it without an thumbs-up in principle from a maintainer.

What you say makes sense to me.

But there might be a more precise solution.

The root problem is likely:

       $is_reusable = $this->isReusable() && $payment_method_type->isReusable();
        ...
        if ($is_reusable) {
          // We cannot attach if the payment is not reusable.
          $stripe_payment_method->attach(['customer' => $customer_id]);
        }

There is one additional source of information present on the payment intent at this point that we might be able to use:
payment_method_options.card.setup_future_usage

Use none if you do not intend to reuse this payment method and want to override the top-level setup_future_usage value for this payment method.

Maybe we can check this property of the payment method options and add it to the logic deciding whether or not the payment method is reusable. If we do this, I think we need to add an ::isResuablePaymentMethod(\Stripe\PaymentMethod $method) method to the gateway plugin, I'm seeing other places where this logic needs to be refined.

Could you check what the value of this property is on one of your Revolut card payment intents that fail?

jonathanshaw → created an issue.

if they return without completing the payment and modify their order, we need to update the PaymentIntent amount accordingly.

@gaydabura thanks for the reply, much appreciated.

(1) I agree that the scenario you mention is valid, and we need to be sure it addressed.

(2) However, I'm suspect that the fix you suggest is not right. If you look at the examples screenshots in payment intent docs at https://docs.stripe.com/payments/paymentintents/lifecycle it illustrates that at the 'require_action' stage the customer is no longer concerned with verifying the amount and may not even be shown the amount. The change you made in OrderIntentSubscriber might conceivably allow other processes (like staff or price changes) to update the payment intent and change the amount charged but without the customer being aware of the final amount they were paying.

(3) Is there are any reason why this issue of customers returning without completing the payment and modifying their order is a bigger concern with paypal than with card payments?

(4) Given that this concern seems to be equally valid for all payment methods, I believe we should open a new issue for it and address it there for all payment methods. The general experience in the Drupal community seems to be that the more problems we try to solve in a single issue, the less likely we are to solve any of them. Smaller scoped issues make for faster progress.

(5) I wonder if the right solution to the problem you give is in the StripeReview checkout pane. Perhaps if it finds an existing payment intent, it should check the amount of intent still matches the order price.

@gaydabura why did you make the change you made to src/EventSubscriber/OrderPaymentIntentSubscriber.php?

That change may have sigificant consequences on payment types other than paypal, so it is unlikely this can be committed with it.

ApplePay is already supported in the current version using the Stripe Payment Element.

There is no Apple Pay payment method type plugin provided by commerce_stripe yet, so that is not exactly true as I understand it. Isn't there still work to do?

I've also found out that because onReturn throws a redirect exception directly, instead of use CheckoutFlowInterface::redirectToStep() it bypasses CheckoutFlowBase::onStepChange() and any logic there.

Again, this non-standard approach has all kinds of side-effects. At the very least it would be good to document why it's done the way it is, so that anyone tinkering with it understands the consequences of changing it.

jonathanshaw → created an issue.

This non-standard approach also has unexpected side effects like 🐛 Order balance is not updated when order placed Active .

jonathanshaw → created an issue.

I think this may be an achievable and desirable feature. The multiple customer ids are a consequence of commerce's default assumption of one remote id per gateway, but there are valid circumstances for having differntly configured gateways on the same provider that share the same remote customer id.

PaymentGatewayBase assumes that the remote id is gateway specific:
$provider = $this->parentEntity->id() . '|' . $this->getMode();

I think our payment gateway plugin could just override get/setRemoteCustomerId:
$provider = 'commerce_stripe_shared|' . $this->getMode();

Backwards compatability here is not trivial as there may be custom code making expectations about the provider id. Unfortunately there is no getProvider() method on the PaymentGatewayInterface to help with this.

We could adopt a permissive approach where if getRemoteCustomerId finds no customer id, it identifies all of the other stripe payment gateway plugins and gets the remote customer id from any of them that have one.

The only one of these edge cases that I actually care about in my project is the anonymous donor whose order can later be identified by having the order email match one of the users emails.

Given that I could workround this by updating the order entity retrospectively to add the customer, I'm sympathetic if you want to say "this contexts business is a nightmare, how about if I create the module without it and then we see if you'v got any budget left to worry about it?"

I'm struggling to understand the idea of "related contexts". I can see that we need to be flexible with context. Staff might detect duplicate donors and combine, or the website might even do it automatically (name/address similar). We might also need to split a donor apart if two individuals got accidentally merged. Or correct an error when a declaration was assigned to the wrong donor in case of two that are similar. However I see all of these as direct edits of the entities, rather than storing a separate entity to record that two contexts are related. I would like to have a goal of 1-to-1 correspondence Donor <=> individual. Could we then remove the idea of related contexts? It would mean that the list of declarations could be found by direct SQL query and we can use standard mechanisms like EVA to show the declarations for a Donor.

Imagine an organisation that has
1. CRM contact entities who may have offline ddeclararions recorded for them
2. Users able to access the website and make or cancel declarations for themselves
3. A commerce checkout where donors can mae a declaration at checkout

This creates a family of nasty edge cases:
A. Not all CRM contacts will have corresponding user entities.
B. A CRM contact may begin without a user but with an offline declaration, but later become linked up with a user and get a declaration or cancellation from that's users actions
C. A donation may be made with a declararion at checkout without being logged in. The user account could be created at the end of the checkout, or it could even never be set as the order customer but still identifiable as the same donor on the basis of email address.

The idea of 'contexts' was an atempt to handle these edge cases. The declaration stores the original context, there's no attempt to dynamically update what's stored on the declaration when further user/contact entitie are added/merged becaus that gets too complex and fragile. But nonetheless the system is able to pull these declarations together for a single donor when needed.

The only one of these edge cases that I actually care about in my project is the anonymous donor whose order can later be identified by having the order email match one of the users emails.

Store the address on the Declaration, correct at time of declaration. This is important if we need to move declarations between donors, or to unpick any errors in combining donors. The Donor address is used whenever we want the most recent known address. HMRC explicitly state that we don't need to update declarations if the address changes.

I've no problem with that as an implementation. I don't have clarity whether it's needed, but I've no problem with it and it feels plausible to me that it's a good solution.

Create an Entity type for Records such as cancellation or change of address. The alternative strategy of using revisions on the Donor is inherently inflexible because they are then locked into the specific Donor. NB I feel the Record entity approach is likely quicker to develop: the extra entity type is balanced by avoiding the need for Donor revisions and we get forms for free from the entity rather than having to design custom forms. In particular Cancellation was a bit strange as a Donor revision because it didn't change any Donor fields; it carried different information which probably we could only store by converting to a string and putting it in the log message.

The idea of a records entity type doesn't worry me as long as we also have a declarations entity type.

I think you probably still want donor revisions anyway as they're the most natural way of handling donor change of address.

I agree cancellation is strange as a donor revision. But there would be an evidence entity that would need linking to the donor as part of the cancelling which might hold a lot of the information.

I agree that a records entity type could well end up a nice clean solution. If you're going this way, I'd suggest not having a bundle entity type for the records. Instead maybe use bundle plugins from the contrib entity module. This means no config needed, and you get a class for each bundle where you can put specific logic, like putting cancellation logic in a postSave() on the cancel bundle.

Sure

I would use the flag so the web types don't appear in the list for staff to create (they only work as evidence for a self-declaration; even if staff create a commerce order on behalf of a donor they would need to use one of the other evidence types). Also the flag could affect access/validity: staff wouldn't need edit-access for 30 days to correct a data-entry error, hence we could allow claims to HMRC without the delay.

FWIW neither of those arguments seem right to me.

You don't need the flag *on the declaration entity* to remove web types in the list for staff. You need it on the evidence bundle. The staff form can show all the bundle types of evidence that have the bundle flag. The self-declare form doesn't overtly mention evidence bundles at all.

The validity argument is worth considering, but that seems like what we have the claimable_date field onthe declaration for. The self-declare form can just set the claimable date to today, the staff declaration recording form sets it to 30 days from today.

For example it seems to imply that Commerce-based declarations need to put the user as context rather than the order (else they will each have a separate donor referencing their separate order). If there is no user, perhaps we put the profile??

I think we better open an issue to explore this. It indicates that we have to be careful to make sure that we don't lose somehting of what the context system offered when we introduce donors.

#3 the evidence entity. I had been imagining a new specific new entity type rather than a dynamic reference to any entity. We can make bundles for each evidence type

Yes.

Probably next we need entity #4 the record that isn't a declaration - at least we need somewhere to store the key data e.g. for a cancellation: date of cancellation, and what does it cancel - a specific declaration (maybe relevant later for non-date-based declarations?) or all?

I'm doubtful we need a record entity type in addition to the evidence entity type. I think revision of the donor and declararion entity types, combined with the addition of new referenced evidence entities, is likely sufficient. This will probably mean things like cancellation need custom forms with a tailor made UX that do bespoke entity manipulation, rather than a simple use of an entity form. But that's ok.

However web and written declarations are fundamentally different evidence types for our software. The first is entered by the donor; has automatically generated evidence that references the order, and the displayed UI text; has corresponding code so is locked in the UI or at least can't be deleted or created. The latter is entered by staff, needs evidence such as a PDF, photo or email, and we should prompt staff to verify its various parts.

I'm not particularly fussed about 2 methods vs 3. But FWIW I don't yet fully share your perspective. Your argument there for me mostly points to the need to have 2 seperate forms, a self one and a staff one. I'm less certain whether that will involve an explicit need to flag web vs written or whether it will be obvious from the fact that declarant = creator.

Perhaps you mean the declared date, which I agree is important and we do have a field for.

Thankyou, you're right.

Create a multi-state validity, merging the two fields, good idea. This becomes very close to the status field, so I'm attracted to drop status as a separate computed field

Agreed. I suspect we need to postpone status until later when we have more clarity over when we need it and what for.

I feel inclined to avoid making the effective date be part of the validity state as that would require a cron hook, batching, and possible error cases

Possibly we will have a computed and a stored validiity (obviously not called the same!).

I will try inline entity form for creating the evidence entity.

I have UX concerns there as per #12.2 above. So by all means investigate, but please don't sink too much time into it. I have found IEF to be one of the most fragile, buggiest, complex and poorly maintained parts of the Drupal ecosystem. So I'm nervous on making this module's complicance critially dependent on significant customisations of it.

More specifically, AccessResult implements RefinableCacheableDependencyInterface so where we handle the query execution we could collect the cacheable metadata from the access result.

Thankyou, now I understand. The proposed queryAccess() handler method allows both modifying the passed in query, and returning an access result with cacheability metadata. Clever, although not a pattern I remember seeing in core before.

In principle having this on the handler seems good. But I see it as a small addition to the current system, not a simplification. And something that could be a separate issue.

I'm interested in the cacheability question you raise, but I don't understand how your handler proposal helps it. Can you say more?

Sorry to be slow to reply.

Replace "written_record_required" with "method" field with 3 states: oral; writing; web. The corresponding state of "written_record_required" is yes; prompt (staff to validate the evidence and tick a box); no.

I feel unsure here. My thinking on this hasn't fully adapted from the old plan where there where different bundles and so staff would encounter a page where they first selected the bundle with an explanation of when to use which, to the new plan where there may not even be declaration bundles and it's the evidence field referencing evidence entities of different type that handles the different delaration methods.

A few thoughts I'm having:

(1) The guidelines distinguish between oral and everything else. And we could actually treat web as a species or written. So maybe the method could just be a boolean is_oral.

(2) How do we create a good UX for staff to select which kind of evidence entity to attach? IEF for example tends to offer just a dropdown with the bundle label, which is not a reliable UX for an important compliance-relevant decision like this. Two possibilities:
(A) use a modal for selecting the evidence bundle when adding evidence. Maybe there's an IEF variant for this.
(B) Reverse the entity creation flow. First the staff choose the evidence bundle from a standard bundle-based entity add page, then they go to a declaration form prepopulated with empty evidence of the chosen bundle.

(3) If we use option B, then we could programatically set the is_oral field depending on the evidence type chosen (if we had an is_oral field on the evidence type, specified by the sitebuilder).

I see that the word record is predominant in 3.10.1 so it's inconsistent! Also A few times it uses "written confirmation". The word "record" is least distinctive, used in many other ways a total of 101 times whereas statement is only used in this context.

"written statement" is used in 3.8 to describe a process for retrospectively validating invalid declarations. "written record" is used in 3.10.1 for what needs to be sent to oral declarants. The two are functionally similar and ideally our work on 3.10.1 should made it easy to support 3.8 in the future, but 3.8 is out of scope for now and a rarer use case. So that's an argument for "written record". "Written confirmation" could be argued to be good because it covers both situations without being overidentifed with either.

DONE Change field "invalid" to "valid". It seems more straightforward, and implies an active choice to demonstrate validity. Optional

Validity is an interesting one. There's 2 main flows in the guidance:
1. An oral declaration is not "effective" if there's no recording until a "written record" is sent. And it its "cancelled" within 30 days of the written record being sent then it is treated as if it's "retrospectively" cancelled and treated as if it were never effective.
2. An declaration with evidentiary or data problems can be deemed "invalid" retrospectively. A charity can "validate" it by sending a "written statement". If undelivered, then treat as "cancelled". If not "cancelled" within 30 days, then the declaration can be considered "valid".

Notably, it's not obviously impossible to send multiple written statements if something weent wrong with the first one, or multiple evidentiary problems were found successively.

So there's 3 states:
1. Invalid/ineffective before written statement sent
2. Pending, will be effective in 30 days
3. No problems, all good

It seems like we might need:
1. An effective_date field that indicates the date from which the declaration can be relied on.
2. A mechanism to set the effective_date to 30 days subsequently when a written record/statement is sent.
3. A way to mark a declaration as cancelled from the moment it was created, being never effective.

Not sure how best to handle all this.

Sure, merge it.

SmartDateDurationFormatter already passes the timestamps.

As I uderstand it: SmartDateDailyRangeFormatter, SmartDateRecurrenceFormatter and SmartDateRecurTrait all seem to built of top of the smartdate field type, which stores timestamps not iso_8601 strings and so already passes timestamps to the augmenter. And as the files aren't opted in to strict types, we don't need to explcitly cast to integer.

So I think we are ok?

The reason why this wasn't done previously is probably this code:

      // If only one of start and end are displayed, alter accordingly.
      if ($parts['start'] xor $parts['end']) {
        if (in_array('start', $parts)) {
          $end_ts = $start_ts;
        }
        else {
          $start_ts = $end_ts;
        }
      }

The code modifies the timestamps permanently, effecting in all the subsequent contexts in which they are used.

Some of these contexts - like the augmenter - need the original stord timestamps, but some of them probably want these modified timestamps. In my fix here, I have passed the stored timestamps to the augmenter only. Figuring out which other contexts need the stored timestamps and which the modified timestamps is beyond me though, that needs deep understanding of this module.

Fortunately, we don't have to solve that here. What I've done in this MR is a step forward, even if there's more that could be done.

jonathanshaw → created an issue.

Does making it optional solve the security issue sufficiently for ✨ Allow users to login using either their username OR their e-mail address Active to advance? The IS needs to explain this aspect I think.

Move the charity and context fields into it (from declaration)

This is the bit I hesitate about.

I think perhaps it needs its own context field, but the declaration still needs a context field too. So the declaration finds its Donor indirectly via the context.

But then from a compliance point of view I guess your thinking its better to be more fixed about the donor's address "on" the declaration. That's a good argument.

But how do we ensure single donor reuse across multiple declarations? By finding any existing donor via context when creating the declaration. So context is still a crucial intermediary. Maybe this is OK.

The name can change and still be the same person.

This point makes me nervous, it would look really bad at audit to be offering a name and adress for a person who was not the one who made the declaration. I think maybe we need a checkbox or confirmation form where they can explicitly choose to apply the new default customer address to their gift aid record.

- Some need it conditionally, depending on 'Advised of liability' (and we can write clear advice to staff for filing out 'Advised of liability')

That's a good idea.

For my legacy paper forms scenario, I can create a bundle specific to that scenario where confirmations are always sent, not optional for staff to decide. It means I need a seperate bundle for new (non-legacy) paper forms, but that's acceptable solution for an edge case like this.

Actually staff shouldn't be creating declarations for the bundles that don't need validation (such as web self-declaration), and we could enforce that (with admin override).

Agreed. We could have permission per bundle, but actually I think it's slightly confusing to use the normal Drupal idiom for something that has compliance implications. Better probably to have a bundle setting "Allow declarations of this type to be recorded by other users." because that way we can add more explanatory notes etc in the UI. The permissions UI is not a good place for stuff that has complicance implications.

My current preferred idea is a hybrid of the 2 solutions: a cancellation would automatically update all declarations to alter their validity. We could separate fields for the original end date (immutable, entered in the UI) and the current end of validity (calculated, read/only in the UI).

I like it. It's very robust and clear. I had hoped to not need it, but it's good and not so hard really.

Note that this "end of validity" is just a "shortcut" which could be recalculated at any stage (someone could write code to do this automatically). This is great for untangling data inaccuracies, such as the "complication" in the IS or if one person has ended up with 2 distinct contexts (database records) which need to be combined.

My concern there is distinguishing between originally specified end dates and later applied ones. So maybe 2 different fields?

In 🌱 Product requirements Active I found this interesting line in HMRC guidance:

3.9.3 If a donor wishes to alter the description of the gift or gifts shown on a declaration [i.e. start and end dates] , they must cancel the declaration and make a fresh one.

I feel we should leave the site owner free to choose how they use declaration types. E.g. they might instead want declaration types to correspond to donation types: one for commerce, one for direct debit, one for a sponsorship form etc. If we have a field for "method" the software can calculate when a written record is required automatically (it would also be true if we are missing some information).

Evidence is required by HMRC - if we have a base field we can enforce that. We could make evidence a reference to a media type, and add a field to say what it is evidence for. Then we could write some code that checked that the current changes that are being saved have matching evidence.

The debate is good, but I stand by my position.

The key point is that what amounts to evidence, and what is a good UI for entering that evidence, is not something we can know. Only the charity will know its existing systems for capturing and storing evidence & how to integrate with them, and what the predominant use cases are for data entry in its situation and how best to optimise UX for them.

We can't know that a media reference field is the best UX for any or most use cases. For example, when we create an email bundle I will suggest that we put fields "email address" and "email text received" on it as that's the best way to create a gift aid trail for an email.

The bundles should focus on optimal UX for different declaration scenarios.

I'm imagining we should provide configured (so optional and alterable) bundles:
- commerce (i.e. checkout)
- web (i.e. self declare by web UI)
- email (staff declare)
- phone (staff declare)
- face to face (staff declare)
- letter (staff declare)
- paper form (staff declare)

The question of when to send a written confirmation is rather complex. I do agree it needs careful discussion. See 🌱 Product requirements Active 3.2.
It looks like site builders should be able to require a written confirmation on a bundle, disallow a written confirmation on a bundle, or make it optional as staff specify.

'Advised of liability' is an interesting one. HMRC seem quite hot on it as part of an audit trail. I have some concern that the fact that e.g. a checkout is configured to show the liability message could provoke a query - can we prove that on such and such a date the user actually saw a liability message and what exact message did they see? It makes me wonder whether it's good even for commerce, web, paper form contexts to have a field that we can set to indicate that the liability advice was given at time of declaration. I'm wondering if even if we prefill it and don't allow staff to untick it on some bundles, it might still be useful at audit and 'look more convincing'.

I have created 🌱 Product requirements Active as a replacement for this issue.

jonathanshaw → created an issue.

The profile type automatically comes with lots of free stuff, including a user tab, however there's a risk it will end up being not quite the stuff that we want and difficult/impossible to alter.

Yes.

So the choice between "Donor entity" and "gift aid profile type" is likely just a matter of ease of implementation.

Yes.

I feel that if we use the existing commerce profile, then we don't actually solve the Problem/Motivation in the IS: profile might get deleted too soon; order might not have an address; revisions are off by default; staff not required to provide evidence; lack of clarity which profiles relate to gift aid.

Good considerations.

I'm definitely open to the idea that it'd be no harder, and likely cleaner or more robust, to have a donor type.

One aspect of handling the commerce customer profile integration could be that if a user edits their default customer profile, then we add a checkbox saying "I have changed my name & tax address for UK Gift Aid too." and update the matching donor correspondingly. Or possibly, we should just assume this is what happens as long as the name is unchanged - HMRC would probably prefer our best guess as to the up-to-date contactable customer address, rather than what we can rigorously prove.

1 and 2 are what I was thinking, great.

I don't fully understand the first and second option in 3. I wonder about having a checkbox when staff create a new declaration with an end date "Enforce this end date on relevant previous declarations?"

we could make a Donor entity type

That's probably got advantages. My hesitation about using a solution other than profiles, is that if one is using commerce, then the customer naturally has a set of already provided addresses in the customer profiles, which it makes sense to allow as options.

My suspicion is that this will be hard to answer until we have a more precise articulation of the exact UI scenarios we need to cover.

Good spot.

Cache::invalidateTags([$old_host_entity->getRegistrationListCacheTag()]);

FWIW I would have done this in Registration::postSave().

jonathanshaw → created an issue.

In \Drupal\registration\RegistrationAccessControlHandler::checkAccess, there is no explicit check to grant access to users with the 'administer registration' permission.

No. But we do have:
$result = parent::checkAccess($entity, $operation, $account);

And EntityAccessControlHandler checks this permission:

protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
...
    if ($admin_permission = $this->entityType
        ->getAdminPermission()) {
        return AccessResult::allowedIfHasPermission($account, $admin_permission);
    }
...
}

But we call that parent check AFTER we check for host/configured.

Yes, Queue::prepareJob() should be able to return NULL, meaning 'discard this job'. This makes sense because prepaeJob() relies on job type plugin's handleDuplicateJobs() method, which has the design feature that it can return nulls, meaning "discard the current because it's the duplicate of an already queued one".

Queue:

  protected function prepareJob(Job $job): ?Job {
    ...
      if ($duplicates = $this->getBackend()->getDuplicateJobs($job)) {
        $job = $job_type_plugin->handleDuplicateJobs($job, $duplicates, $this->getBackend());
      }
    }
    return $job;
  }

JobTypeInterface:

  /**
   * Handles existing jobs detected as duplicates when enqueing a new job.
   *
   * A function can be used to execute a range of different strategies with
   * regard to duplicate jobs:
...
   * - to discard the new job and leave the duplicate intact, return NULL.
...
   * @return \Drupal\advancedqueue\JobResult|null
   *   A new job to enqueue on the backend, or null if no new job should
   *   be enqueued.
   */
  public function handleDuplicateJobs(Job $job, array $duplicates, BackendInterface $backend): ?Job;

I think your concerns about audit are worth taking seriously for sure. Maybe revisions and Entity Reference Revisions for the profile reference field would address it. Or just store the address *at the time of declaration* on the declaration in an address field that doesn't get updated.

When you make a Gift Aid claim you don't show the declarations to the Inland Revenue or break it down by declaration. Instead the claim is specific to a financial year and shows the donors name, address and the total amount of donations in the year for that donor that you're claiming Gift Aid on. The address used should be the best address for the donor that you have, even if you're making a claim for 3 years ago when they gave you an older address.

So the concept of address is really more at the level of the donor rather than the declaration, even if it needs to be present on the declaration.

By cancelled, I mean to set the end date to now. I believe this solves the problem you refer to.

That sounds like my second solution from the IS?

I don't feel intuitively connected to your idea that there is a special active declaration, ... It seems that at best it could only apply ... at a certain moment in time... Mostly we will want to know if at least one declaration exists, i.e. hasActiveByAnyContext(). We might also need to put a link to one of the declarations, but it could potentially be chosen arbitrarily??

Good. Our thinking aligns. Ironically we each thought the other was saying the opposite!

create a new declaration when rights are granted, i.e. the date is moved later, and bulk update when rights are removed.

Yes!

In terms of retraction, then I could see that as a special case to handle by a user with admin permissions.

A good enough solution for now to be sure.

In the case of declarations moving into a context (a online donor with existing old declarations gets associated with a CRM contact who has recently cancelled gift aid)

I wonder if this module needs to take responsibility for that case??

Agreed. I think it's good to have some awareness of context edge cases in case we can sometimes avoid them. But let's not create elaborate mechanisms to handle them. Nice if we can document them.

I think we have the theoretical clarity we need, we can move on to implement.

As I pointed out before, this function is currently not used, so I feel it's premature to keep spending time altering it.

Agreed. Can you leave a todo flagging the question and otherwise leave the code in whatever state you think best?

john.oltman → credited jonathanshaw → .

I don't think it's particularly important, but thought the idea might be worth sharing.

jonathanshaw → created an issue.

See also ✨ Record donor name & address Active

Nice!

2) Sure I can wait if you like. However in my view, the current code is unacceptable in terms of audit requirements as declarations change their end date in baffling ways without an audit trail. We could safely remove it even if we don't know what we'll use instead.

For right now, I don't mind which you do. But the MR is weird in that it reintroduces $later, but also removes $newer. That will definitely produe bad results I would think. Originally bother were present.

(3) getActiveByAnyContext

If 2 declarations had the same end date, we should prefer the newer of the 2.

(5) The dates in Declaration entity.

I'd have had 3 concerns with the MR:
- it used \Datetime to get the current time, not Drupal's time service. That's untestable and not best practice.
- it seemed to remove the edge case handling associated with this comment:
// The declaration ends at the end of the day, not beginning of the day.
- I thought that anytime we stored a date in Drupal we needed to update the timezone to the storage timezone before we format it using the storage format. Because when it gets loaded it will be assumed that the stored string is in the storage timezone. Seems dangerous to break that assumption for any custom code accessing these fields.

I like your idea that maybe assuming Europe/London is a good rule of thumb in this module. And your point that the tax year end is in BST might be important.

My current thought is that getStartDate() and getEndDate() should rebuild the timezone to Europe/London:
$formatted = $stored->format('Y-m-d H:i:s');
$new = new DateTime($formatted, new DateTimeZone('Europe/London'));

Additionally, getEndDate() should use "23:59:59" as the time.

The donor has changed their mind. We can then mark the old declaration as cancelled

mark the old entry outdated: this is an updated preference

I don't think we can do either of these things. The problem is that declarations will often be partially overlapping: the old declaration may have a start date earlier than the newer one, and in that earlier period it may be the necessary declaration for donations made in that period; it's only superseded in the later period.

The basic questions with cancellation is "where do we store the new end date"? On the declaration we currently think is the active one, or on all of the relevant stored declarations? I'm thinking it's safer to apply it to all of them, in case some shenanigans changes which one is active.

Changes of start date should be very rare, and are hard to reason about; I'm happy to leave that to admins to figure out manually editing all the relevant declarations.

I feel it will help us to phrase things in terms of the standard Drupal terminology of CRUD.

I've actually been skittish about this. As you say, donors shouldn't be exposed to the complicated concept of declaration instances. But for things like cancellation, I'm not sure that non-admin staff should be either. We might need some simple forms that take the necessary entity updates, but which ones are conventional entity edit forms is not yet clear to me.

Creating a duplicate fails

I don't think so. It's a new source of legitimacy to a Gift Aid claim, even if Inland Revenue decided at audit that your previous declaration was invalid for some reason, you could still be covered if this new supposed duplicate with a different evidence trail was valid.

Update is allowed within a short time period to correct a clerical error and after that, there are access restrictions.

Yes, I've wondered about whether we would end up with a mechanism like that. Not yet fully clear to me. I guess there's always scope to upload additional evidence, so maybe the access control is at the field level.

Can cancel (set end date to 'now') but cannot set end date in the past. Maybe can also shorten or lengthen end date to a new future date

I'm thinking that it may be better on GiftAidOverviewContextController to show the user's current Gift Aid declaration status, and then have buttons like "Add declaration" or "Cancel" that go to specific forms with a UI and staff guidelines appropriate for that.

Delete is not allowed except for admin

Except within short time period as you suggest above.

jonathanshaw → created an issue.

Yes, if we use this for the access it makes sense to have it.

Feel free to reopen if you think that's right, I agree we definitely should have an issue for adding UI for customers to add/manage payment methods and it would be great to see your code for this.

(1) You're right, the removal is good.

(2) This is related to 🌱 The problem of changes Active . I'm OK with you removing the code here if necessary to pass tests, or leave the todo to resolve in the context of that issue.

(3) Consider this scenario. A user makes a declaration saying we can claim gift aid indefinitely. Later a user makes a declaration seperately saying we can claim gift aid this year. Does the later more limited one win out over the earlier one here? I was thinking it shouldn't because it might be an artifact of a more narrowly focused form or something, but now I doubt that and think maybe the newer one should be taken as a partial cancellation per 🌱 The problem of changes Active . So I suggest we need to revisit this question in that context, but the removal may well be OK. However, even if the removal is technically OK because the circumstances should not arise, I'm not sure whether or not it's good to use this point-of-loading to enforce the cancellation.

(4) The only thing that's making me hesitate about the fields is the commerce store, which has some of the same information. Probably that's just harmless duplication and we should put the fields in. Yes, fine to use address module.

5)

I fixed the start/end dates on Declaration, however on reflection I believe it's still not right.

I'm not sure what you mean. Maybe what you did in
https://git.drupalcode.org/project/gift_aid/-/merge_requests/3/diffs
I overlooked that earlier, there's various things about it that don't seem right to me at first glance.