Dansday

The Month I Opened the Same PR Three Times

Published on Jul 31, 2024

Ten pull requests in July. Five merged. Five were closed without ever landing, and three of those five were the same piece of work attempted three different ways on the same afternoon.

February had been the month I replaced the framework underneath a live shop. July was the month I did it again, having learned enough to know what it would cost.

The tail of June

The first nine days of July were June arriving late. The promotion refactor, the category banners, the FAQ reshuffle, the analytics change — four pull requests opened in June that merged between 1 and 9 July. This is why counting a month by what you opened tells you almost nothing about what you shipped.

One small follow-up on 2 July fixed the order of components on the static pages, six additions and four deletions in a single file. The previous week I had moved the store list, the customer stories and the FAQ block around on those pages, and I had moved them into an order that read fine in the diff and wrong in the browser. Six lines to put them back.

Reverting my own analytics

On 9 July the GA4 tracking merged: the checkout button reporting which shipping and payment method the customer had chosen, alongside the product.

On 12 July I reverted it. Two additions, ten deletions, three files, in a pull request called simply Revert #557.

On 15 July I put it back. Ten additions, two deletions, three files.

I have compared those two diffs and they are exact inverses — the restored code is byte-for-byte what the revert removed, down to the indentation. Whatever came out on the twelfth, it was not because the code was wrong, because nothing about it changed before it went back in on the fifteenth. Six days, three pull requests, and a net change of nothing except the state of master in between.

The shape of that code is also not mine, and I want to record that accurately. My first attempt read the selected methods in the cart controller and passed them through. The review said no:

Just directly use `app.util.trackClick` on your blade files.
No need to put in cart, that is redundant.

My reply in the thread was a genuine question — How to get the value then if moving into the blade? — and the answer was to read the DOM at click time, which is how it ended up as three chained calls inside an onclick:

app.util.trackClick('{{ EventActions::ACTION_SHIPPING_METHOD }}',
    document.querySelector('input[name=delivery-option]:checked').id);

That review was right about the redundancy and I still do not love where it landed. Reading .id off a :checked query returns null when nothing is selected, so this is correct only while the form guarantees a default. It works. It is guarded by a convention rather than by the code, and conventions are not enforced by anything.

Three pull requests, one job

On 11 July I opened three pull requests for the same upgrade.

Bagisto v2.2.1                     1,427 files   closed the same day
Upgrade bagisto v2.2.1             1,368 files   closed 1 August
Upgrade bagisto fix implementation 1,405 files   merged 23 July

That looks like indecision and it was actually method. An upgrade of this size is not one change, it is a sequence of choices about how to reconcile two trees, and the cheapest way to find out whether a reconciliation strategy works is to build it and look at the diff. The first one I killed within hours. The second I left open as a reference while the third — the one that separated the mechanical version bump from the fixes it required — went through review.

It took twelve days to merge. The review thread has five rounds of comments from one colleague, four responses from me, two dismissed reviews, and finally three approvals. Nothing about the process was fast and nothing about it should have been.

Why an upgrade is 1,405 files

The reason a version bump touches fourteen hundred files is architectural, and it is the thing I would explain in an interview.

This application does not depend on Bagisto. It is Bagisto — the framework's own packages/Webkul/ tree lives in the repository, committed, with our customisations layered alongside it in a separate namespace. That is the shape Bagisto ships in. It also means an upgrade is not a line in a manifest, it is a merge between our copy and eight months of upstream commits, resolved by hand.

The admin settings tree alone came to 646 additions and 144 deletions in one config file. And upstream had reorganised whole areas: a 203-line customer address grid deleted outright, replaced by three new per-customer grids for orders, invoices and reviews totalling five hundred lines. An 86-line combined invoices-and-transactions grid split into separately renamed grids for invoices, transactions and shipments.

None of that is code I wrote. All of it is code I had to read closely enough to know whether our overrides still made sense against it. The February article said the work in an upgrade is in the parts that break; by July I would put it differently. The work is in the parts that moved, because a file that was deleted upstream and is still referenced in your code fails at runtime, not at merge time.

One dependency addition is worth pointing at:

+ "mpdf/mpdf": "^8.2",

February's invoice — the 434-line printable PDF I was proud of — renders through whatever PDF library the framework provides. Upstream changed it. That is the recurring tax of building on somebody else's platform: the thing you shipped keeps working only as long as you keep paying attention to the floor underneath it.

What QA found

The interesting part of those twelve days is the bug list, because it is the map of what a large upgrade actually breaks.

Custom data had gone missing from the order view — our additions to a screen upstream had rebuilt. Transaction actions were appearing on only some rows, which a colleague caught by recording a video of the grid and asking why order #818 could do something the others could not. And original_price and base_original_price were no longer being included, which matters because those are the fields the customer's savings figure is derived from — the same numbers I spent December making honest.

All three are the same category of failure. Not a crash, not an error page. A field that used to be populated and now silently is not, on a screen nobody looks at closely until a customer asks a question about their order.

A migration with an asymmetric undo

The upgrade needed one data migration, and I would write it differently now:

public function up()
{
    DB::table('order_transactions')->update([
        'payment_method' => DB::raw('type')
    ]);
}

public function down()
{
    DB::statement('
        UPDATE order_transactions ot
        JOIN order_payment op ON ot.order_id = op.order_id
        SET ot.payment_method = op.method_title
    ');
}

Upstream had started storing the payment method on the transaction under a different column, so every existing row needed backfilling from the old one.

Two things about it. The up() has no where clause, so it rewrites every transaction in the table including any that were already correct — harmless here, and exactly the sort of unscoped write that stops being harmless the moment the table has rows the assumption does not cover. And the down() does not reverse the up(); it reconstructs the column from a different table entirely. If the two sources ever disagreed, rolling back would not restore the previous state, it would compute a new one. In February I wrote about scoping a rollback carefully so the undo stayed honest. Five months later I wrote this one, under time pressure, in the middle of fourteen hundred files.

Then v2.2.2, briefly

The upgrade merged on 23 July. On 24 July I opened a pull request for v2.2.2 — 1,092 additions, 564 deletions, 114 files. It was closed on 1 August without merging.

The day after that, a staging fix for the version we had just shipped: 386 additions, twelve files, opened and merged the same day. That ordering is the whole lesson. Reaching for the next version the day after the last one landed, while the last one still needed a staging fix, was me mistaking momentum for progress.

Hunting queries with a tool instead of a hunch

The last pull request of July is the one I liked most. I opened it on 31 July and it was closed on 28 August without merging — but the code did ship, which I only worked out afterwards. The same branch was reopened on 13 August under a different ticket, filed as CMS editor work, and merged the next day: identical file set, identical line counts. The original was closed a fortnight later as the duplicate it had become. So this is in production, bundled with a rich text editor, because both changes lived on one branch and the CMS ticket was the one moving.

In May I had found an N+1 problem by reasoning about it — nine methods, nine queries, twenty products, a hundred and eighty round trips. That works when you already suspect where the problem is. It does not find the ones you have not thought about. So this time I installed something that tells you:

+ "beyondcode/laravel-query-detector": "^1.8",

A development-only package that watches requests and warns when a loop starts issuing queries. Then I went where it pointed. The similar-products block on every product page was loading a category's products and then lazily fetching each one's attributes, images and variants:

$products = $mainCategory->products()
    ->with([
        'attribute_family',
        'attribute_values',
        'variants.attribute_family',
        'variants.attribute_values',
        'images',
        'promotionVariants'
    ])

Six relations declared up front instead of discovered one product at a time. The same treatment went onto the attribute options, which had been resolved per attribute in a loop:

- foreach ($this->super_attributes as $attribute) {
+ $superAttributes = $this->super_attributes()->with('options')->get();
+ foreach ($superAttributes as $attribute) {

And two lines I fixed on sight, because I had been bitten by their cousin in March:

- if (count($this->categories) === 0) {
+ if ($this->categories->isEmpty()) {

- $mainCategory = $this->categories[0];
+ $mainCategory = $this->categories->first();

Indexing [0] on a collection that might be empty is the trade-in list bug from March, in a different file, waiting. first() returns null instead of throwing. Four months earlier that pattern cost me an afternoon; in July I recognised it in passing.

The monitoring script stopped checking certificates

One commit outside the main repository, on 31 July, called Disable ssl. Five additions, two deletions, in the Python URL checker:

+ import urllib3
+ urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)

- response = requests.get(url, headers=headers, timeout=30, allow_redirects=False)
+ response = requests.get(url, headers=headers, timeout=30, allow_redirects=False, verify=False)

Two request calls stopped verifying TLS certificates, and the warnings that would have told me so were switched off in the same commit.

The reason is almost certainly in the article above. July was the month of the framework upgrade, and this project builds a throwaway QA environment per branch at its own subdomain. Certificates on short-lived environments are exactly the thing that is not properly issued, so a checker that refuses to talk to them is a checker that reports everything as broken.

The trade I made is still worth stating plainly, because I did not state it at the time. A monitoring script that ignores certificate errors will report a site with an expired certificate as perfectly healthy. That is precisely the outage it would be most useful for catching. I turned it off to stop the noise on staging, and in doing so I turned it off for production too.

What July was

Ten pull requests, five merged, and one line that quietly weakened my own monitoring. A framework upgrade that took three attempts and twelve days of review, a version after it that I was too eager about, an analytics change that left and came back unchanged, and a query detector pointed at a page I had never suspected — which reached production a fortnight later under somebody else’s ticket number.

Half of what I opened in July was closed unmerged, though as it turns out not all of it was wasted — some of it shipped in August wearing a different number. The upgrade is the kind of work with no screenshot — nobody at 3cat could tell you it happened, and the invoice, the promotion system and the checkout all kept working across it. That is the whole deliverable: a shop that noticed nothing.