[gonzaloebiz]

list_member_count has been plumbed through lib_hot, lib_domain and lib_domain_daily since it was built and has never held a real value — the library only ever observed the account root, so the legacy estate could report an account-wide figure and nothing per audience.

What it does

observeList() mirrors observeRoot(): passive, on a response the caller already has. The extension's statistics job reads lists/{id} every twelve hours, so no call, no quota, no latency. It ignores anything that is not a single audience — the collection endpoint answers with lists and no stats.

getLists() gains include_total_contacts, appended last so every existing positional caller is unaffected, and off by default.

Why total_contacts and not the sum

member_count + unsubscribe_count omits non-subscribed contacts, and on an ecommerce account that is 96–98% of the total — 413 against 20,971 on one account in the estate. So the sum is not an approximation of the billable figure, it is a different number.

total_contacts is absent unless asked for, and absent is left as null rather than derived. A fabricated total that nearly matches is worse than one that is missing, because nobody checks a number that looks right.

total_contacts includes cleaned — measured, not assumed

On an audience with 631 cleaned:

total_contacts	1,767
subscribed + unsubscribed + cleaned	1,752
subscribed + unsubscribed	1,136

1,767 sits above the sum that includes cleaned, not near the one that excludes it.

So the docblock tells a consumer deriving the non-subscribed residual to subtract cleaned explicitly — 15 on that audience rather than 646 — and that even then the residual is an upper bound on transactional, since pending and archived land in it and archived are never billed.

Verified against the live API

case	result
single list, without the parameter	latches the audience, member=3 unsub=0 cleaned=0, total_contacts null
single list, with the parameter	same plus total_contacts=5
collection lists?count=1	everything null — ignored
root	everything null — different family

Envelope, with independent instances so no bucket leaks between cases:

full : {"list_id":"AAA","total_subscribers":10,"list_member_count":1120,
        "list_unsubscribe_count":1,"list_cleaned_count":631,"list_total_contacts":1767}
lean : []
without total_contacts : the three present fields, the absent one omitted

And the guard that keeps the numbers attached to the right audience: a response for lists/AAA arriving while the bucket has already latched BBB is ignored, rather than reporting one audience's identity with another's counts.

tools/no-deprecated-calls.php: 64 files, 0 findings.

Field names, and the half that is not here

The names mirror the existing list_member_count column rather than inventing a second vocabulary — say the word if the receiver would rather have different ones and I will rename before this is tagged.

This delivers nothing on its own: total_contacts only arrives if a caller asks, and the extension's GenerateStatistics currently calls getLists($listId) bare. That is one query-string argument on the extension side, queued separately so it rides the next extension release rather than being remembered.

[dipola]

Approving. The measurement work behind it is the part I would not have got right on my own — you tested the parameter on both endpoints, and you settled the cleaned question on real data rather than reasoning about what total_contacts ought to mean.

The latch guard is the detail that matters most and it is the right way round: first audience wins, a different one is ignored rather than overwriting, so we never publish one audience's identity with another's counts. And leaving total_contacts null when the caller did not ask, instead of deriving something from the other three, is the whole argument of this thread honoured in code.

One finding: the single-audience test is the response shape, not the path

family() returns lists for every lists/... path, and harvest() returns a list_id for any path whose second segment exists — so lists/{id}/members/{hash} harvests {id} just as lists/{id} does. The only thing separating them is isset($result['stats']).

The collection case is genuinely covered twice (no second path segment, so $listId is null and it returns before the stats check). Sub-resources are covered once, by a property of Mailchimp's responses rather than by anything we asked for.

And there is a real collision underneath it. A member object has its own stats:

GET lists/{id}/members/{hash}  ->  { "id": …, "email_address": …,
                                     "stats": { "avg_open_rate": …, "avg_click_rate": … } }

So observeList() does fire on every member fetch during a subscriber sync — a hot path. It writes nothing today, because a member's stats carries none of the four keys, so every isset() misses. Harmless, and it even latches the list id usefully. But the correctness rests on Mailchimp never adding a key named member_count, unsubscribe_count, cleaned_count or total_contacts to a member's stats object — and if that ever happened it would write one subscriber's numbers into the audience fields, silently, with the right-looking shape.

One line closes it, and makes the guard say what you actually mean:

$parts = explode('/', trim($path, '/'));
if (count($parts) !== 2) { return; }   // lists/{id} only, never a sub-resource

Not a blocker — your call whether it rides this PR or the next. I would take it, because everything else here is guarded on what we asked for rather than on what came back, and this is the one place that is the other way round.

Field names: keep them

list_member_count is not just consistent with the column, it is already on the receiver's whitelist — so that one starts landing the moment you tag, with no edge change at all. The other three are dropped today and I am adding them along with their columns; that is my side and it does not gate you.

The half that is not here

Agreed, and worth saying plainly for whoever reads this later: until the extension passes the parameter, this ships the three counts that are not the billable figure and omits the one that is. member_count + unsubscribe_count is 2–4% of the truth on an ecommerce account. So the extension one-liner is not a nice-to-have that can drift — it is the half that makes the other half mean anything. As long as it is queued against the next extension release rather than remembered, we are fine.

tools/no-deprecated-calls.php clean, lean envelope empty, independent instances per case — all checked.

[gonzaloebiz]

Taken — b623f6c. You were right that it is the one place guarded on what came back rather than on what
we asked for, and that is a good enough reason on its own.

I checked the collision against a real member before adding the guard, rather than reasoning about what
a member object ought to contain:

harvest('lists/{id}')                  -> list_id '…'
harvest('lists/{id}/members/{hash}')   -> list_id '…'     ← same id, as you said
harvest('lists/{id}/members')          -> list_id '…'

a real member's stats keys: avg_open_rate, avg_click_rate, ecommerce_data
collides with the four we read? not today

So it was firing on every member fetch and writing nothing by luck rather than by design.

Then the case that matters, with a member whose stats carries member_count = 999:

path	before	after
lists/{id}	member_count=3, latches AAA	unchanged
lists/{id}/members/{hash} (poisoned)	would have taken 999	refused
lists/{id}/members	—	refused
lists/{id}/interest-categories	—	refused

And nothing is lost by refusing them, which I checked because your note said the member path latched
the id usefully: record() already calls harvest() on every call and latches list_id there
(:444-450), first-non-empty-wins. So a member fetch still contributes the audience id exactly as
before — the guard only stops it reaching the counts.

On the field names

Good to hear list_member_count is already on the whitelist — that means the tag alone starts landing a
real value in a column that has never held one, with no edge change. Keeping all four names as they are.

On the half that is not here

Agreed, and your phrasing is the one I would keep: until the extension passes the parameter this ships
the three counts that are not the billable figure and omits the one that is. It is queued rather
than remembered — mc-magento2, PR 2365, open against develop-2.4, one argument at
GenerateStatistics.php:195, verified both ways on a running install. It does not need this tagged to
merge; it needs this tagged to mean anything.

[dipola]

Re-approving on b623f6c — my earlier approval was recorded against 03cc764, so it was stale against the new head rather than absent. This one is on the current commit.

Checked the guard rather than taking it: 17 lines, four of them code, the rest the reason; the file still parses; and the path set behaves —

lists/abc                                parts=2  OBSERVE
lists/abc?include_total_contacts=true    parts=2  OBSERVE
/lists/abc                               parts=2  OBSERVE
lists/abc/members/hash                   parts=4  refused
lists                                    parts=1  refused

Poisoning a real member with member_count = 999 is better than what I asked for. I raised it as a property of Mailchimp's responses we do not control; you turned it into a case that fails before and passes after. That is the difference between believing a guard works and knowing it does.

And checking that record() still latches list_id from harvest() on every call closes the one thing my note left open — I said the member path latched the id usefully, and you confirmed refusing it costs nothing because the latch happens elsewhere. I had not verified that; you did.

One nit, not a blocker and not reachable today. harvest() strips the query string before splitting; the new guard does not. They agree on every path the library actually builds, because getLists() produces 'lists/'.$id with no trailing slash — but lists/{id}/?x=1 splits into three and would be refused. A false negative, so the safe direction, and unreachable from any current caller. Worth one line only if you are in the file anyway.

On PR 2365

Good — queued rather than remembered, which was the whole of my concern. Your last sentence is the one I would put on both: it does not need this tagged to merge; it needs this tagged to mean anything. Send it over when you want it looked at.

Tag whenever you like. The receiver takes list_member_count today, and the other three plus their columns are mine and are not a gate on you.

[dipola]

@gonzaloebiz the harvest works — member_count, unsubscribe_count and cleaned_count come through correctly keyed and typed, and total_contacts comes through null, exactly as you set out in the PR body. Confirmation rather than a bug.

Two things for whenever you pick this up, neither urgent:

1. mailchimp/mc-magento2#2365 touches only Cron/GenerateStatistics.php. The admin config page call sites (Model/Config/Source/Details.php:81 and Model/Config/Source/MonkeyList.php:42) call getLists() with a single argument, so with the PR's current scope the field would only ever arrive on the cron path.

2. Mailchimp's schema marks include_total_contacts as deprecated in the same sentence that documents it, and points at the /audiences endpoint for a complete audience contact count. Do we treat this as a bridge and aim at /audiences later, or go there directly?

No rush.

[gonzaloebiz]

Both checked against the live API rather than the docs.

1. It is five call sites, not two — and the consequence is on your side

getLists() is called seven times in the extension. One is a collection (Lists/Get.php:60, which
observeList() refuses anyway). Five fetch a single audience without the parameter:

Controller/Adminhtml/Stores/Get.php:62
Controller/Adminhtml/Ecommerce/Getaccountdetails.php:82
Model/Config/Source/Details.php:81          <- you named this
Model/Config/Source/MonkeyList.php:42       <- and this
Helper/Data.php:1120

All five are admin, web lane — and the web lane is unsampled, so each one emits a beacon carrying
list_member_count, list_unsubscribe_count, list_cleaned_count and list_total_contacts absent.

So the question this raises is not really about scope, it is about your merge rule: does a later
beacon with total_contacts null overwrite a good value from the cron? If it is last-wins, an admin
opening the Mailchimp config page nulls the billable figure until the next cron run, twelve hours later.
If it is first-non-empty-wins per field, nothing happens and the extra call sites cost nothing.

I would rather ask than widen the extension's scope on a guess. If overwriting is a risk, the cheap fix
is on my side — pass the argument at all five — and I will do that rather than have you special-case a
null.

2. /audiences gives the same number, and costs a call

Probed on a real account:

audiences/{id}                        -> { id, name, stats: { total_contacts: 5 }, enabled_channels }
lists/{id}?include_total_contacts=true -> stats.total_contacts = 5, member_count = 3

Same figure. But two things follow:

/audiences/{id} carries only total_contacts. Its stats has that one key — no member_count,
no unsubscribe_count, no cleaned_count. So it cannot replace the lists/{id} call, it can only be
added to it.

Which means "go there directly" costs one extra API call per audience per run — and the passivity
is the entire reason this was worth doing. observeList() was justified on riding a response the cron
already had; a second call for one field is a different proposal that has to argue for itself on the
merchant's quota.

So my answer to your question: treat it as a bridge, and do not aim at /audiences until we are made
to. While include_total_contacts works it is free; the day it is removed the cost becomes real and
the decision should be re-made then, with the price visible, rather than paid now against a deprecation
notice that has not bitten.

Worth recording that the deprecation is in the schema rather than in behaviour: nothing in the response
warns, and the field returns a correct value today. The trigger to revisit is the parameter failing or
disappearing, not the notice existing.

[dipola]

Your concern is real, not hypothetical — an incomplete reading can displace a complete one on our side. That is ours to fix, not a reason to constrain the scope of this PR.

Please do pass the argument at all five. It is the better fix on its own terms regardless of what we change: it makes every reading complete, which is what the handling was designed around. I would rather that than special-case an absent value and end up holding figures read at different moments as if they were one.

We will harden our side too, so an incomplete reading cannot displace a complete one even from a call site nobody anticipated. Your five are the ones we know about; the rule should not depend on knowing.

Agreed on the bridge, and your reasoning is better than my question was. The deprecation is in the schema rather than in behaviour — the trigger to revisit is the parameter failing, not the notice existing. And /audiences/{id} carrying only total_contacts settles it on its own: it adds a call rather than replacing one, which is the opposite of what made observeList() worth doing.

Good catch on the call sites. I had two of five.

[gonzaloebiz]

Done — mc-magento2, PR 2366, all five passing the argument.

Controller/Adminhtml/Stores/Get.php
Controller/Adminhtml/Ecommerce/Getaccountdetails.php
Model/Config/Source/Details.php
Model/Config/Source/MonkeyList.php
Helper/Data.php

Verified by the shortest of the five — constructing the admin audience dropdown:

	list_member_count	list_total_contacts
before	3	null
after	3	5

Controller/Adminhtml/Lists/Get.php is left alone: it fetches every audience rather than one, and the
beacon ignores collection responses, so asking would add payload nobody reads it from.

One sequencing point, since last-wins makes it matter. This has to ship in the same extension release
as the cron change, not after it. The cron alone would put complete readings twice a day against
incomplete ones on every admin page load — which is the state your rule handles worst. So the tag that
delivers any of this is the one carrying both.

[dipola]

Checked the five files on 2366 — all there, and Lists/Get.php correctly left alone for the reason you give.

Your sequencing point is the important part of this comment, and you are right. The cron change alone would be worse than shipping neither: complete readings twice a day against incomplete ones on every admin page load, which is exactly the case that goes wrong. Same tag for both, or neither.

Worth saying so it does not sit as an unspoken dependency: we are hardening our side so an incomplete reading cannot displace a complete one regardless of what any caller does or what order anything ships in. That work does not change your conclusion — the same-tag rule is right on its own terms, and I would rather it held even if our side were perfect, because the next call site nobody anticipates should not be able to cause this either.

Two of five was my reading of the code; five of five was yours from the live API. Second time today. Noted.