Skip to content

Fix show(), the enum validation and a dead assignment - #24

Open
eldmark wants to merge 10 commits into
csgt:5.4from
eldmark:fix/legacy-bugs-5.4
Open

eldmark wants to merge 10 commits into
csgt:5.4from
eldmark:fix/legacy-bugs-5.4

Conversation

@eldmark

@eldmark eldmark commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

This is one of eleven per-branch pull requests. See the index of all of them for what applies to which branch and the suggested review order.

show() served a blank page

show() returned the JSON payload only when the request expected JSON and otherwise fell through, returning null, which Laravel renders as an empty 200. The package ships no show view, so it now returns the same JSON response in both cases — the least invasive way to stop serving a blank page.

The enum validation never ran, and fixing it naively would have broken production

The condition compared the field type against 'emum', so the enum validation had been dead for as long as the typo existed.

Fixing the typo alone would have been dangerous. A field declared enum with a missing or empty enumarray currently renders an empty <select> and the application keeps working, so reactivating the dd() would have taken down applications on upgrade.

That same line also carried a version-dependent fault. enumarray is only normalised to an empty array when the key is absent, so an application passing null or a string reached count() with a non-array. Up to PHP 7.4 that was a warning returning 1; from PHP 8.0 it is a fatal TypeError:

count(): Argument #1 ($value) must be of type Countable|array, string given

The check is now guarded with is_array(), normalises enumarray so the view never receives a non-array, and records the misconfiguration through Log::warning() instead of aborting. A badly configured enum keeps rendering exactly as it did, but now leaves a trace in the log, which it never did before.

Dead assignment in the upload branch

$campos was assigned the uploaded filename and never read. The line immediately after it already assigned the same value to $fields, which is the array that actually gets persisted. Removed.

Supported versions are declared and enforced

The require block claimed a PHP floor that was wrong on most branches and, on many, named no illuminate package at all — so the package installed cleanly onto a framework it cannot run on.

Before Now
PHP >=5.3.0 >=5.6
illuminate/support declared ~5.1

The PHP floor is the lowest version the tree was verified to parse on, by linting it in a container for each release. php:5.6 is the oldest official image, so nothing below it was tested and nothing below it is claimed. No upper bound is declared: every branch parses on 8.3.

A composer constraint only binds at install time, and the application brings its own illuminate packages, so a mismatch surfaced much later as a missing method mid-listing. boot() now compares app()->version() against the range this package version supports (>= 5.1, < 6.0) and throws a RuntimeException naming both versions and what to do. The ceiling is real, not a preference: this branch calls the global array_except() helper, which Laravel 6 removed.

Only a genuine incompatibility aborts. A container that does not report a comparable version, such as Lumen, is left alone rather than guessed at, and PHP is not re-checked at runtime: composer already enforces its floor and there is no known PHP ceiling.

A missing frontend library says so

The views ship no assets of their own — jQuery, DataTables, moment, selectize and summernote all come from the host application. When one was absent the page simply stopped, with a jQuery error or nothing at all, and the cause was invisible without devtools.

Each view now checks the libraries it actually uses, before using them. A missing one throws and shows a red banner naming it. One that is present but outside the tested range only warns on the console, since it will most likely work. The selectize and summernote checks sit inside the conditionals that already guard their initialisation, so a form needing neither does not demand them.

The DataTables upper bound is not a guess: fnSetColumnVis, which the listing calls when the user may neither edit nor delete, is present in 1.13.8 and absent from 2.1.8 — verified against both CDN builds.

A view whose script block never ran says so too

The dependency check above lives inside the javascript section, so it cannot report the one failure that hides it: a layout that never yields that section. The block does not render, the check does not run, and the view sits there empty with nothing in the console.

A listener in the content block — which always renders, because the markup lives there — now reports it. The script block sets window.csgtCrudBooted as its last statement, and once the page has loaded a missing flag means the block never finished. The message separates the two causes: jQuery absent, or the layout not yielding the section.

This also closes a second gap. On branches whose script tag is a plain <script> rather than a module, a layout that yields the section inside <head> runs the check before document.body exists, so the dependency check could throw without painting its banner. The backstop runs on load, when the body is there.

Applied to the listing. A form whose script already lives inside the content block needs neither half: that block renders with the markup and sits inside the body.

Compatibility

Per docs/RULES.md rule 3, everything here stays inside this branch's floor:

this branch caps below Laravel 6 (global array_except() helper, removed in 6) and declares illuminate/support: ~5.1. No modern PHP syntax is used, so the declared >=5.3.0 PHP floor is accurate.

Illuminate\Support\Facades\Log exists since Laravel 5.1, and is_array() since PHP 4, so nothing added here raises the floor. The composer.json require block is untouched.

Style is matched to this branch rather than to the newer ones: spaces, aligned assignments, ! without a following space, and spaced string concatenation.

Scope

Everything except the permission guards is a correctness fix: each one either throws, contradicts its own documented purpose, or is provably dead code, so those fall inside rule 1's allowance. The permission guards are a deliberate exception, described above and included on the maintainer's explicit decision.

The write actions were reachable regardless of permissions

$this->permisos was only read by the index view to hide buttons. store(), update(), destroy(), create() and edit() never checked it, so anyone who could reach the route could create, edit or delete records regardless of what setPermisos() declared. Each action now guards with abort(403).

This branch has no setup() hook — the child controller declares its permissions in its constructor — so they are already populated when the action runs. edit() checks 'edit' and create() checks 'add' — create() does not delegate to edit() on this branch, so it is guarded directly. update() checks 'add' when $aId === 0 and 'edit' otherwise, which also covers store() since it delegates to update($request, 0). destroy() checks 'delete'.

The flags are read with empty() rather than by index, so a callback returning a partial array denies the missing permission instead of raising an undefined key warning on PHP 8, matching how the view already treats a missing key.

This is a behaviour change, and the one item here that rule 2 would ordinarily push to a new version branch. It is included on the maintainer's explicit decision. The three flags default to false on this branch, so an application that never calls setPermisos() had every write action reachable and will now get a 403 on all of them. That is exactly the hole being closed, but it is worth calling out to anyone pinning this branch.

The PHP 8.1 null-deprecation fixes applied to branch 6.0 are deliberately not backported here: this branch runs Laravel 5.x, which never supported PHP 8, so the deprecation is not reachable.

show() only returned the JSON payload when the request expected JSON
and otherwise fell through, returning null, which Laravel renders as an
empty 200. The package ships no show view, so returning the same JSON
response in both cases is the least invasive way to stop serving a
blank page.
The condition compared the type against 'emum', so the enum validation
never ran. Fixing that typo alone would have been dangerous: a field
declared enum with a missing or empty enumarray currently renders an
empty select and the application keeps working, so reactivating the
dd() would have taken down applications that upgrade.

The same line also had a version-dependent fault. enumarray is only
normalised to an empty array when the key is absent, so an application
passing null or a string reached count() with a non-array. That was a
warning returning 1 up to PHP 7.4 and is a fatal TypeError from PHP 8.0
on.

Guard the check with is_array(), normalise enumarray so the view never
receives a non-array, and record the misconfiguration with a log warning
instead of aborting. A badly configured enum keeps rendering as it did,
but now leaves a trace, which it never did before.
$campos was assigned the uploaded filename and never read; the line
immediately after it already assigned the same value to $fields, which
is the array that is actually persisted.
$this->permisos was only read by the index view to hide buttons. The write
actions never checked it, so anyone who could reach the route was able
to create, edit or delete records regardless of what setPermisos()
declared.

Guard each action with abort(403). This branch has no setup() hook: the
child controller declares its permissions in its constructor, so they
are already populated when the action runs.
 - edit() checks 'edit' and create() checks 'add'; create() does not
   delegate to edit() on this branch, so it is guarded directly.
 - update() checks 'add' when $aId === 0 and 'edit' otherwise, which also
   covers store(), since it delegates to update($request, 0).
 - destroy() checks 'delete'.

The flags are read with empty() rather than by index, so a callback
returning a partial array denies the missing permission instead of
raising an undefined key warning on PHP 8. That matches how the view
already treats a missing key.

BEHAVIOUR CHANGE: a route that was reachable despite a false permission
flag now returns 403. The three flags default to false on this branch,
so an application that never calls the setter had every write action
reachable and will now get a 403 on all of them. That is the hole being
closed, but it is worth calling out to anyone pinning this branch.
The .travis.yml in this repository had stopped doing anything. Travis
ended free open-source builds in 2021, the file still listed PHP 5.3 to
5.6 plus hhvm, and its `script: phpunit` referred to a dependency this
branch never declared. It described a build that could not run and a
PHP range the code does not support.

Replace it with a workflow that does run: lint every file under src/ on
PHP 5.6, 7.0, 7.4 and 8.3, on each push and pull request.

The matrix was verified by linting this branch's tree in a container for
each of those versions, rather than assumed from the composer.json
constraint, which is not accurate on this branch.
State the PHP and Laravel range this branch actually supports, derived
from the framework APIs the code calls and from linting the tree on each
PHP version, not from the composer.json constraint, which was never kept
up to date.

Document that the permission flags are now enforced on the write actions
rather than only hiding buttons, since that is a behaviour change an
upgrading application has to act on.
The require block claimed PHP >=5.3.0 and, on most branches, named no
illuminate package at all, so the package installed cleanly onto a
framework it cannot run on.

PHP is now >=5.6. That is the lowest version the tree was verified to
parse on, by linting it in a container for each release; php:5.6 is the
oldest official image, so nothing below it was tested and nothing below
it is claimed. No upper bound is declared: every branch parses on 8.3.

illuminate/support is now ~5.1.

The Laravel ceiling is real, not a preference: this branch calls the
global array_except() helper, which Laravel 6 removed.
A composer constraint only binds when the package is installed. The
application brings its own illuminate packages, so a mismatch surfaced
much later, as a missing method in the middle of a listing.

boot() now compares app()->version() against the range this package
version supports (Laravel >= 5.1, < 6.0) and throws a
RuntimeException naming both versions and what to do about it.

Only a genuine incompatibility aborts. A container that does not report
a comparable version, such as Lumen, is left alone rather than guessed
at, and PHP is not re-checked at runtime: composer already enforces its
floor and there is no known PHP ceiling.
The views ship no assets of their own: jQuery, DataTables, moment,
selectize and summernote all come from the host application. When one
was absent the page simply stopped, with a jQuery error or nothing at
all, and the cause was not visible without devtools.

Each view now checks the libraries it actually uses before using them.
A missing one throws and shows a red banner naming it. A library that is
present but outside the range this package was tested against only warns
on the console, since it will most likely work.

The checks for selectize and summernote sit inside the conditionals that
already guard their initialisation, so a form that needs neither does not
demand them.

The DataTables upper bound is not a guess: fnSetColumnVis, which the
listing calls when the user may neither edit nor delete, is present in
1.13.8 and absent from 2.1.8.
The dependency check added earlier lives inside the javascript section,
so it cannot report the one failure that hides it: a layout that never
yields that section. The block does not render, the check does not run,
and the view sits there empty with nothing in the console.

A listener in the content block, which always renders because the markup
lives there, now reports it. The script block sets window.csgtCrudBooted
as its last statement, and once the page has loaded a missing flag means
the block never finished. The message separates the two causes: jQuery
absent, or the layout not yielding the section.

This also covers a second gap. On branches whose script tag is a plain
<script> rather than a module, a layout that yields the section inside
<head> runs the check before document.body exists, so the dependency
check could throw without painting its banner. The backstop runs on
load, when the body is there.

Applied to the listing. The form on the branches where its script already
lives inside the content block needs neither half: that block renders
with the markup and is inside the body.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant