🇬🇧United Kingdom @jonathanshaw

Tests pass now, test coverage requested in #12 added, no outstanding feedback. I think this is ready for RTBC.

Rebased.

I agree that #58/59 is important, and I suggest someone add it to the MR and test it.

if there is a hook_install shouldn't there also be an accompanying update_hook for those that already have the module installed

Agreed, added #77 to MR.

The checkout flow plugin was too aggressive in forcing the checkout flow step to 'order_information', it was overriding the step specified on the order which disrupted checkout flows relying on that, such as the Stripe Payment Element for example. Fixed.

Once the customer has paid the retry payment, the access check on the route tells them "You are not authorized to access this page."
These links remain in emails and get clicked after their intended goal has been completed.
It might be more graceful to respond with a message of "Already paid" or maybe redirect to an empty /cart?

Ideally yes. Perhaps could be follow up.

I suggest you spilt these fixes into different issues, so they can be discussed seperately. A combined patch like this is rather unlikely to get committed, progress usually happens faster in smaller more tightly scoped issues.

I'm not sure what's going on with the tests, I think the fails are unrelated.

Sorry for the delay in replying Adam, I was away.

I understood the plan you laid out above in #9, yes. It was a good plan. I thought it was rather elegant.

But what I found when I interrogated the edge cases was that this plan was not completely robust in all circumstances. In some of the edge cases it caused extra complexity that seemed likely to outweigh its simplicity. As if it was doing too much with too few building blocks, in a way that was elegant in the common use cases but got confusing and tricky in edge cases; and we likely end up having to think about edge cases even if we don't implement them.

We can tell the staff's assessment either that validity was reached by means of a confirmation vs inherently valid by checking if there is a "confirmation sent" date.

This is a nice example. In the most obvious use cases that works fine. But what happens when people want to send confirmations even if not strictly necessary? Then it's not possible to use the fact of the confirmation to make a conclusion about the staff's assessment of the validity.

All I'm suggesting here is a simple change:
Confirmation required, and not yet sent (= confirmation missing)
becomes
Confirmation required (whether or not sent)

And instead of programatically changing that validity field when confirmation sent, we have a computed method of some kind instead.

I appreciate that potentially causes downstream problems for things like views that might want to query by validity, but I'd rather we deal with those problems (likely by storing the computed value in an additional field, a simple solution) when we know better what the problem is there that needs to be solved. For now, it seems to me that storing the human-provided data in the most granular way possible creates the most robust foundation for the various needs.

(p.S. I confess storing retraction as a validity state is a bit weird and not entirely in keeping with the spirit of some of the other things I said above, but it seemed to work in all the edge cases I considered. Happy to leave that aside).

Sorry, cross-posted with you.

I'm not attracted to the idea that interface concepts are different from the underlying state - especially isValid(). I don't like the idea that invalid is different from a negation of valid, as I feel that's confusing.

I'm sympathetic to both of those points. But we do need to distinguish between:
- the staff's evaluation of the validity, which we don't change programatically
- the overall effective validity considering the staff's evaluation and the known facts about confirmation
then we do end up with something like what I've suggested here.

Basically the staff need to choose between "Is this declaration valid without confirmation, valid only with confirmation, or totally irredeemable?", and be allowed to change their minds. But in various contexts the system needs to know if the declaration is good, (e.g. when establishing do we have a working declaration for this donor), which requires computing something based on considering both that staff evaluation and the sent or not sent confirmation.

We could call that isUsable() and isNeverUsable() instead of isValid() and isInvalid().

Then in the roadmap we have a bullet like this:
Written statement ...
It would make sense to me to turn this into an issue which again I would class as a feature request

Done: ✨ Written confirmation Active

I feel that cancellation is an important, big, complex feature. I wouldn't want to mix it with anything else, I wouldn't want to do it in parts, I would do the other issue in entirety when we reach the appropriate point.

Makes sense. And I agree that the retraction related stuff in this MR can be split out.

Which leaves us with the residue of this MR, which basically boils down to this:
Although there's nothing in the code currently that explicitly changes the validity field when confirmation is sent or 30 days after confirmation is sent, the fact that one of the validity constants is named DECLARATION_CONFIRMATION_MISSING and the isValid() method is only concerned about the DECLARATION_VALID constant, suggests an expectation that the validity field does get updated at some point.
And I've realised that this is problematic: it's better to leave the validity field to store the staff's judgment about the validity (which they could change their mind about) and then have more sophisticated logic in isValid() to create a computed judgment of validity that combines the staff's assessment of the evidence with the systems knowledge of the confirmation.

So this is technically a feature ( ✨ Written confirmation Active ) we haven't yet, but we have existing code that seems to make assumptions related to it.

I could update the IS and MR here accordingly (removing retraction) if you think sensible. It would effectively then be a first stage of ✨ Written confirmation Active .

jonathanshaw → created an issue.

I implemented #21.

Splitting into successor issues:
📌 More robust validity Active
🐛 Declaration:status problem Active

📌 More versatile mechanism for controlling editability of declarations Postponed
✨ Add suspended field Postponed

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

I suggest we remove all code related to declaration status for now. I'm not clear enough what problem this is trying to solve, to know what constraints the solution needs to obey.

The code we can remove is:
Declaration::statusOptions()
Declaration status field
DeclarationInterface status constants
Drupal\gift_aid\Plugin\Field\DeclarationStatusItemList

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

For the 2 web self-declare cases, the explanation text isn't in the evidence, so we would need the bundle field.

Or we follow 📌 Change handling of web evidence Active and additional evidence for the web cases to capture the explanation.

I don't really have a good sense of the best solution here, base field, bundle field or evidence. My slight inclination is to think bundle field is the right solution, because it makes it clear that the field is only relevant to those bundles that use it.

If the bundle field was configurable, maybe other declaration types could add it smoothly if it was right for them.

I had also considered that we could track the confirmation email wording or template version.

I think it would be a good idea to have a "confirmation email" evidence type that has the date sent, address sent to, full text, etc. and always capture that.

I needed the cacheability enhancements so I finally got around to this :)

I have never installed the necessary for infrastructure for FunctionalJavascript tests. Probably we need just one test, so it's not really efficient for me to learn it. Would you be willing to do it yourself please?

In DeclarationFormTest::testOralDeclarationRecordedByStaff(), the return statement should be down to just before @todo Fix when added code for cancellation.. It just needs some ajax to add evidence, which might start something like

$this->assertSession()->waitForElementVisible('named', ['button', 'Add new Gift Aid evidence'])->press();

Maybe best to make this a seperate issue? I can do it.

Fix deprecation notices in test results (although some come from Commerce tests?)

Whenever you like

Combine the 2 Commerce sub-modules? People might forget to enable both. I've never enabled gift_aid_commerce_product, and it has no tests. Is it needed?

I'm on the fence.

gift_aid_commerce is totally neutral about how you specify which orders need to seek a gift aid declaration. You could just put the pane on a particular donation checkout flow and have it always show there. gift_aid_commerce_product allows you to opt products in to saying they are donations and should seek a declaration if the order includes them, which is useful if the same checkout flow includes both donations and other products.

gift_aid_commerce_product is still opt-in per product even when enabled, so it could be combined. But it does add a base field to the commerce product entity type, which is a slightly invasive operation.

I'm on the fence. I kind of like having submodules for atomic features, I think of it as aiding maintainability but that's a matter of taste.

Move the menus one level down, e.g. put Gift Aid under Services? This is the normal level, and it means that the various admin pages for the module are all visible as tabs.

Sure, no opinion.

Have doubts about compliance - it doesn't appear to be an optional item. We might believe/hope that the audio recording contains a spoken record of the explanation. What if the staff member forgot? What if the donor was shown a card to read showing the explanation? I feel that it would be safe/clear to have a field that forces the staff member to check. It could be a bool, but that's easy to tick out of habit without checking. So the robust answer is to get staff to actually listen for the explanation wording.

I have wondered about things like this. For example, whether the declaration is valid depends on it having 5 key pieces of information, and an explanation, and evidence. One could imagine all kinds of UIs to get staff to verify the presence/absence of all the key ingredients and behind the scenes compute validity based on that.

However, the conclusion I've come to is that the right UI depends enormously on the particular circumstances of the declaration. So it's best left for sites that want this to set it up using custom fields/widgets on a custom declaration type specific to their organisation's workflow. As you point out, it's dangerously easy for any UI to be largely ignored by staff on autopilot, so it needs to be carefully suited to the circumstances.

What the guidance says is "in order for a Gift Aid declaration to be valid, the charity must give and be able to demonstrate it has given an adequate explanation to the donor of the personal tax implications associated with making a Gift Aid donation".

3 things of significance here:
- to "demonstrate" an explanation has been given sounds to me exactly like evidence, which we already have a mechanism for with the evidence field.
- in 99% of cases that evidence of the explanation will be the same thing (paper, audio recording, web form) as the evidence of the declaration
- the explanation could actually be on the confirmation email, so it could be missin initially

Therefore I don't think "explanation" makes sense as a base field on the declaration, because it's rarely necessary to treat evidence of explanation sperately from evidence of declaration & confirmation, and sites that want to do something more elabarate for a particularly tricky declaration type easily can.

Assigned to J to document ideas for "Alter some declaration fields, converting multi-state values to multiple booleans"

Addressed in 📌 Declaration states Active

However then it hits a bug with missing parameter context, because the parameter is inherited from the user account, which is user.

So if I understan right you're saying that this code works for the route:

gift_aid.user_context:
  path: '/user/{context}/gift-aid'
  options:
    parameters:
      context:
        type: entity:user

but it breaks the links.task because that expects the parameter name to match the base route its inheriting from. Interesting, makes sense.

This has been horribly difficult to work on, and involved much more change than I expected, but I think I ended up with an approach that's a step forward. It should also make life much easier when we come to implement cancellation and confirmation, as it's much more robust to edge cases involving them.

jonathanshaw → created an issue.

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.