🇬🇧United Kingdom @jonathanshaw

jonathanshaw → created an issue.

It just occurred to me, if we are doing that, we could equally well put the properties on the evidence type. So to revisit the TLDR of #4, we need 2 bits of "funky work", whether or not we have declaration types.

I take your point, but I still lean in favour of declaration types.

I'm becoming persuaded of your point in the declaration types issue, that if we have those then we don't need this evidence type at all.

Postponing this issue on that for now.

Surely if we are having declaration types, then we no longer need web evidence

Fair point.

We might want it anyway for cancellation and COA?

Thanks for all your engagement in this. I definitely made a few mistakes in my thinking along the way that you've cleared up.

A related wrinkle is that sites will likely want to link anonymous checkout declarations with donor by order email even if the order is never assigned.

The "related contexts" stuff could help with that in theory but it's a bit heavyweight.

The label is equal to the description on the evidence type. The URL could be determined.

I'm imagining 'web' evidence would be recorded for self declaration, self cancellation, and self COA (which could itself possibly have multiple triggers).

So my idea for URL is to help distinguish these.

The label is redundant, and we can easily skip it for now. I think it might make the evidence trail easier to understand at a glance but in truth it's probably a premature idea at this point.

About user, I think I've been getting confused and imagining web evidence being generated by staff submissions. Which isn't actually in our current plan.

Arguably the question of providing evidence of how we knew who the submitting user was is still relevant for audit, but I think we can safely ignore that as it is inferable from circumstances.

Originally I had explanations on evidence. I moved it because evidence is also used for cancellation and CoA which don't need explanations. If we put it back then we need to handle that.

What I'm thinking is:
if there's a recording or scan the explanation is captured in that file, or with an email it's captured in the email text. So it's in the evidence anyway for offline declarations.
It's only self-declarations on the website where it'd be good to capture the precise explanation used. In which case putting it on the associated evidence type seems sensible.
It's true the explanation field on the web evidence type wouldn't make sense when used for COA/cancellation. But as this type of evidence is always programatically created, not exposed to users, that hardly matters.

2) Please can you explain why?

I'm seeing this as an evidence type that is relevant to any kind of form submission or programmatic generation of evidence: declarations, cancellations, change of address.

The term "Web" is currently used as the method for all self-declarations.

I suspect it won't be once we've finished refining our ideas in ✨ Declaration types Active .

- User: already present via the reference to Donor, but I guess we could put it again.

I'm thinking of this with a belt-and-braces audit hat: it'd be good to have evidence about who was submitting a form, even if we currently think that the form will only be used for self-submission with a user donor context.

I guess the counter argument is that we already have this in the revision_user field on the revision. That's a strong point, but I wonder if we get better audit UX more easily if add it on the evidence type too.

- URL: currently the URL is entirely determined from the User. This is only needed if we use one evidence type for multiple scenarios, which I don't understand the advantage of.

If this evidence type is used for cancellation and COA as well, then it involves different urls. Additionally, future use cases or custom code could create new forms and want to create evidence associated with their use. So adding the exact url seemed like a good audit idea, rather than relying on the assumption that the evidence could only be generated in one circumstance.

- Label of what?

I was imagining that it might help to provide a human-readable explanation of the context that generated the evidence: "Checkout", "User self-declaration", "User change own address". Might make the display of evidence in the evidence field easier to make sense of.

3) Sure, we could, but I don't really understand why. It seems to be entirely duplication of information, which I'm not especially fond of (what if staff edits one place but not the other?).

I agree we have a choice with commerce order. Either
(a) Add the url (and maybe authenticated user) field(s) to the order evidence type to enrich the audit trail there
(b) Add 2 pieces of evidence when creating an order declaration.
I'm not sure which is better.

I'm seeing a lot of value of (multiple) evidence entities, even in the context of declarations:
- as discussed in 📌 Change handling of web evidence Active it's a nice way to generically store audit information about a web submission, such as the explanation given
- it's a nice place to store details of any written confirmation sent (when, to where, what wording)

For clarity:
what I'm suggesting is that a declaration would still have an evidence field, but we'd label it "Additional evidence". The declaration type fields would handle the main original evidence for that declaration, the evidence field would handle supplementary stuff. The declaration add form woudn't even need to show the evidence field.

Not necessarily "quite a few fields" but one field about allowable use cases (declare/cancel/CoA) should be enough.

I take your point, a set of checkboxes for use cases would work.
If they checked the "declare" use case then there'd need to be a show/hide #states on a second setting about whether written confirmation is always/never.

I retract my point #1 entirely; I'm not sure if it's an easier or harder mental model but it's OK either way and sitebuilders aren't a priority in some sense.

I don't think it's harder having declaration types because I think most sites will be happy with just 3 simple evidence types (for cancel/COA/misc) that we could provide OOTB (email, document (pdf/image), and oral). It's declaration types that I'm seeing a lot more likelihood for sites to want to make custom flavours of as they will be much more heavily used.

2. Using the current IEF widget would be hard. We'd need to restrict it to just this subset of evidence types when creating a declaration; and even then the current dropbutton only shows the label and doesn't allow for a description. Likely we'd need to cobble together a non-standard UI to require staff to select one of these when creating a declaration. That could be a selection page followed by a querystring param that's interpreted in the declaration form, or it could be a customisation of the IEF widget or some custom widget.

Yes there would need to be some restrictions. But it's likely not as bad as the restrictions needed if we have both declaration and evidence types.

This UX problem is the big one for me.

Regarding the restrictions, there's no problem if declaration and evidence type is seperate. We would then have a declaration "add page" where you select the type first, no need to involve evidence types.

The worse problem is how we show a bundle description to the user if we're just using an IEF. Customising IEFs gets hairy fast.

In various ways you would like the evidence types to influence fields that are actually present on the declaration.

Exactly. Which is why it feels more natural to me for this to be the "declaration type" that is influencing the declaration fields. If we only had one piece of evidence per declaration this'd be less of an issue, but for reasons given above and below I think it's valuable to retain the possibility of multiple.

If we are going to have declaration types then potentially we should remove the evidence entity, or at least stop using it for declarations

I like the evidence entity, for the reasons given above. It does a great job of handling edge cases and weirdness because it's s flexible. It's just slightly weaker at creating simple well-trodden pathways for the main everyday staff scenarios.

Declarations would generally have just one evidence I expect. HMRC are clear I believe - any change should be a new declaration. Editing is only allowed for corrections.

Allowing multiple pieces of evidence on an entity does a good job of handling weird edge cases. Imagine if a declaration gets given as part of an email conversation with a donor - when is that conversation completely over? What if the donor sends another clarifying email? What if a form is hard to read so it gets scanned a second time for improved legibility? etc. What if staff email a donor to check something with them because they couldn't quite read a form? etc

Your evidence system is wonderfully flexible and I'm keen to keep that.

=============

TLDR;
the choice is between either
(1) Do some funky work to make it work with evidence types:
(a) either (i) have a custom IEF widget, limited to 'declare' use cases evidence types and showing description as well as label; or (ii) have an evidence type select page (like a normal entity type add page) before the declaration form and use that to preselect the evidence type open in the IEF simple widget in the declaration form.
(b) use that first piece of evidence as the key 'type' of the declaration for influencing other fields and make sure we only care about that when the declaration is first submitted, make sure we don't need or depend on that after that in case the evidence gets messed with by an admin.

(2) Add declaration types, with a declaration add page to select the declararion type before the declaration form. The declaration types do overlap somewhat with evidence types, but everything else about them is straightforward idiomatic Drupal.

Either approach works. My suspicion is that (2) is easier.

I can see why for us, thinking about the concept of evidence in the context of the HMRC guidance, it seems weird to have evidence stored in these two places. But this distinction also corresponds with an important one in practice:
- 95% of the time staff would use the system just add declarations in a simple way
- dealing with other "evidence" would only come up in much rarer circumstances (my organisation basically almost never deals with COA or cancel evidence in practice).

Regarding the change from DonorAddForm to DonorAccountForm the basic idea is that when I looked at it I realised that it could be more generic than just the user account scenario. It would work with any type of context entity, once I added the DonorInterface::isCurrentUser() method. A site would need to customise the donor entity class to make it work with non-users, but it could be done in principle. It seemed to me like it was turning into a pretty generic Donor 'add' form.

jonathanshaw → created an issue.

Most commits are trivial. The DonorAddForm commit contains a substantial rework of things related to ContextOverviewController, DonorAccountForm and related logic. I like where this ended up, something more generic that works with contexts other than the user.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

I've provided a fallback to manually convert paths to absolute, copying what s3fs decided to do in #3264640: S3fs does not handle paths with './' or '..' the same as core. → . Sorry for somewhat hijacking this issue, hopefully you agree these concerns are closely entangled.

Sure, pls do an alpha

Actually I was too hasty. The problem with stream wrappers that don't support realpath already exists and this doesn't worsen it.

But it could fix it:

$base_dir = $this->fileSystem->realpath($scheme . '://');
if ($base_dir) {
  $target = $base_dir . '/' . $path;
}
else {
  $target = $result->uri;
}

I'm not sure why we're using realpath here, but we should at least have a fallback when it's not available. My use case is s3fs.

+1

This makes S3 files show up in reports for me.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

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.