Fix hadith reference URLs with letter suffix (#104) - #183
Conversation
wakqasahmed
left a comment
wakqasahmed
left a comment
There was a problem hiding this comment.
Verdict: approve (posting as a review comment — GitHub will not let this account formally approve its own PR).
Cold-start review (no prior context on this change). I verified the root cause independently against the live site rather than taking the description on trust.
Root cause: confirmed
https://sunnah.com/abudawud:4290bcurrently returns 404, whilehttps://sunnah.com/abudawud:4290returns 200. Bug reproduced.- On
https://sunnah.com/abudawud/38the rendered reference for that hadith isSunan Abi Dawud 4290 b(with a space) while the permalink the page itself emits ishttps://sunnah.com/abudawud:4290b(without the space). So the site links to a URL its own lookup cannot resolve. - The asymmetry is explained by the two normalizations in
ArabicHadith.php:populatePermalink()strips the space unconditionally for every collection (preg_replace("/(\d)\s*([a-zA-Z])/", "$1$2", ...)), whereaspopulateReferences()applies the samepreg_replaceonly when$collection->name == "muslim".Util::getURNByNumber()likewise only had the space-insertion retry inside the$collectionName === 'muslim'branch. The PR description's account of the root cause holds up exactly.
Regression analysis: no regression found
- The new block is placed after the direct-match
if (!is_null($direct)) { ...; return null; }, so it is only reachable when the direct lookup returned zero rows. Common numeric lookups (abudawud:4290) are untouched and pay no extra query. - The regex is anchored (
^(\d+)\s*([a-zA-Z]+)$), so plain numeric input never enters the block. Multi-letter suffixes are covered by+. - Sahih Muslim: the new block now runs before the existing muslim branch and computes the same
"N x"string. For the single-match case the outcome is identical to the old muslim path. For the multi-match case the old code fell through to the"<input> a"heuristic and the partial search, both of which could not match a spaced entry anyway, so nothing that previously resolved stops resolving. The pre-existing "no letter supplied -> try adding 'a'" heuristic is untouched.
Scope
Diff is a single file, application/modules/front/models/Util.php, +17/-1. No unrelated changes. Leaving the Muwatta Malik short-URL request out is the right call — it is a separate feature (malik currently has no colon permalinks at all; sunnah.com/malik:445 404s and /malik/1 emits only /malik/N links), not a bug fix.
Blocking-ish nit before merge
The PR body says Fixes #104, which will auto-close issue #104 on merge — but #104 contains two requests and the Muwatta Malik one is explicitly deferred. Please change it to Refs #104 / Partially addresses #104 so the malik half is not silently closed.
Testing
composer.json carries codeception dev deps, but there is no tests/ directory and no .github/workflows/, so the PR's claim that no automated test covers this path is accurate. Correctness here therefore rests on reasoning plus manual verification — flagging that as a review consideration, not a blocker, given the repo's existing test culture. See the inline comment about extracting the repeated status-check block; that refactor would also give this logic a unit-testable seam if the maintainers ever want one.
Approving. The change is narrow, correct, and matches the surrounding style.
| // If the hadith is not found and the number has a trailing letter suffix | ||
| // (e.g. "4290b"), the stored hadithNumber may have a space before the | ||
| // suffix (e.g. "4290 b"). Retry the lookup with the suffix separated out. | ||
| preg_match('/^(\d+)\s*([a-zA-Z]+)$/', $hadithNumber, $suffixMatches); |
There was a problem hiding this comment.
Regex reviewed for unintended matches: it is anchored at both ends and restricted to digits + letters, so (a) a plain numeric hadith number can never enter this branch, and (b) no LIKE metacharacter can reach searchByNumber(). That second point matters here because searchByNumber() calls andWhere(['like', 'hadithNumber', $x, false]) — the false disables Yii's escaping, so a % or _ in the input would otherwise become a wildcard. The regex guarantees $spacedNum is metacharacter-free, so this adds no new wildcard surface. Good.
One dead-ish branch: the \s* allows an already-spaced input, in which case $spacedNum equals $hadithNumber and you re-issue the identical query that just failed. Unreachable in practice — the route pattern is <hadithNumber:\w+>, which cannot contain a space — so this is cosmetic only.
| preg_match('/^(\d+)\s*([a-zA-Z]+)$/', $hadithNumber, $suffixMatches); | ||
| if (count($suffixMatches) === 3) { | ||
| $spacedNum = $suffixMatches[1]." ".$suffixMatches[2]; | ||
| $direct = $this->searchByNumber($collectionName, $spacedNum); |
There was a problem hiding this comment.
Suggestion (follow-up, not blocking): consider $num = $spacedNum; when the spaced retry finds nothing, so the spaced form also reaches the multiply-numbered fallback further down.
As written, $num keeps the unspaced "4290b", and the fallback compares trim($resultHadithNumber) == $num against comma/dash-exploded stored values. A row stored as "4290 b, 4291" would therefore still fail to resolve from /abudawud:4290b, because "4290 b" != "4290b". I could not find such a row on the abudawud book-38 page, so this may well be hypothetical today — but it is the one suffix case this fix still leaves unhandled, and propagating $num closes it for one line.
| $book = $this->getBook($collectionName, $result['bookID'], "arabic"); | ||
| if ($book->status >= 4) return $result['arabicURN']; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Worth being explicit about this early return null: if the spaced lookup finds rows but none of them belong to a book with status >= 4, the function returns immediately and skips both the muslim-specific handling and the multiply-numbered fallback.
I traced whether that loses anything and concluded it does not — in every such case the downstream paths would have been searching with the unspaced number, which by construction cannot match a spaced stored value. So this is behaviour-preserving. Noting it only so the intent is on record: the early return is deliberate, and it mirrors the identical early return in the direct-match block above.
| if (!is_null($direct)) { | ||
| foreach ($direct as $result) { | ||
| $book = $this->getBook($collectionName, $result['bookID'], "arabic"); | ||
| if ($book->status >= 4) return $result['arabicURN']; |
There was a problem hiding this comment.
Nit / optional: this foreach + getBook() + status >= 4 + return-first-published block is now the third near-identical copy in this method. A small private helper — e.g. firstPublishedURN($collectionName, $results) returning the URN or null — would remove the duplication and, more usefully, give this method a pure, unit-testable seam (relevant since there is currently no test covering this path at all). Fine to leave as-is if the maintainers prefer to keep the diff minimal.
|
Updated the PR body: switched |
1 participant
Refs #104
Summary
Hadith reference URLs with a letter suffix (e.g.
sunnah.com/abudawud:4290b) returned a 404, even though the hadith exists and is displayed on its book page (e.g.sunnah.com/abudawud/38).Root cause
Util::getURNByNumber()looks up a hadith by exact match against thehadithNumbercolumn. For letter-suffixed hadith numbers, the stored value has a space before the suffix (e.g."4290 b"), while permalinks and user-typed URLs use the space-free form ("4290b", see thepreg_replace("/(\d)\s*([a-zA-Z])/", "$1$2", ...)normalization already used when generatingpermalink/canonicalReferenceinArabicHadith.php). The direct lookup inUtil::getURNByNumber()never re-tried with a space inserted, so it fell straight through to "not found" for any collection other than Muslim, which already had its own special-cased space-insertion logic.Fix
Added a general retry step in
Util::getURNByNumber(): when the direct match fails and the requested hadith number matchesdigits + letters(e.g.4290b), retry the lookup with the letters separated by a space (4290 b) before falling through to the multiply-numbered-hadith case. This applies to all collections, not just Muslim, and doesn't change the existing Muslim-specific handling (letter-detection retry and the "no letter supplied -> try adding 'a'" heuristic), which remains in place as-is.Out of scope
This PR deliberately does not implement the second request in the issue (a new short-URL scheme for Muwatta Malik, e.g.
sunnah.com/malik:445). That is a separate feature request and is left for a follow-up PR.Test plan
abudawud:4290bnow finds the row stored ashadithNumber = "4290 b"via the new suffix-retry step and resolves throughgetURNByNumber->front/collection/urnthe same way a normal numeric lookup does.Util/hadith-number resolution), so no test file was changed.