Conversation
… results easier for humans
…suppressing Ruby deprecation warnings
|
CI tests failed, perhaps because the coverage dropped The baseline will need to be adjusted. |
…s passing on coverage numbers, add test to cover scenario lost with forms removal
Author
This was 100% correct, and it has been fixed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CMS-434 Remove the Forms subsystem
Deletes BrowserCMS's built-in Forms subsystem: 34 files removed, 14 modified, 2 added — 1,948 deletions / 156 insertions across four commits.
Forms has been broken since 5 June 2017 and is unused. This PR removes it rather than repairing it. No migration, no schema change.
It also carries the upgrade-plan corrections the removal forces, and two test-output conveniences found while verifying it (
script/test-summary,test/quiet_warnings.rb). Those last two are independent of the removal and reviewable on their own — commit86d5f21din isolation if you want to read them separately.Why remove rather than fix
It is unused in production. Verified 17 Sep 2026 against
ricksteves-prod:A local restore of production current to 2026-08-29 is likewise empty across all four tables (
cms_forms,cms_form_fields,cms_form_entries,cms_form_versions), in a database with 5,328 pages and 1.5M connectors.It is unreachable in the UI. The app hides the type in two places:
app/views/layouts/cms/_content_types.html.erb(rse/cms) overrides the engine's nav with a hardcoded allowlist of 29 content types.Cms::Formis not on it.app/views/cms/connectors/new.html.erb:15listsCms::Forminskip_types, so it cannot be added to a page.There is no link to any Forms page anywhere in the admin. You could only reach one by typing the URL.
Nobody ever did.
FormsController#newcalled@block.save!before rendering, so every visit to/cms/forms/newpersisted a row and then 500'd before the cleanup JS could run. Zero rows means the page was never successfully loaded in the years that database covers.The site's real forms are elsewhere. Pages under
/forms/*are ordinary pages built fromRse::ArticleandCms::HtmlBlockembedding Cognito Forms -- a<script>tag pasted into the editor body, stored as HTML inarticles.content/cms_html_blocks.content. As of the 2026-08-29 restore, 14 content records embedcognitoforms.com(13 articles + 1 HTML block, all sharing onedata-keywith a differentdata-formper page), across 14 pages; four more records contain hand-written<form>markup. Several pages are named "Form Test - Jotform / Formstack / Wufoo / Google", which reads like a vendor bake-off that the CMS subsystem lost.What was actually broken
Five defects, all traceable to two commits on the same afternoon:
b2e3df44(2017-06-05) "removes addressability form forms to prevent creation of dynamic routes" — commented outis_addressable path: '/forms'23c10f70(2017-06-05) "removed routes for forms" — commented out two routesCms::Form.layoutundefined —form_entries_controller.rb:17and:31PathInputcallsobject.class.path;Cms::Formhas none —forms/_form.html.erb:7,path_input.rb:14forms#newand#edit500preview_form_field_pathundefined, route commented out —forms/_form.html.erb:30entries_pathundefined, route commented out —forms/_form.html.erb:64,forms/show.html.erb:3FormEntriesController#createand#bulk_updateCms::Formhad nopage_title/ancestors/landing_page?, which page templates call throughcurrent_pageDefects 3–5 were invisible until the earlier ones were fixed — each exception masked the next. Two of them, #3 and #4, are load-bearing:
previewis the only way to add a field to a form, andentriesis the only way to read a submission. So Forms could not build a form or show a result, and had not been able to for eight years.Two more artifacts of the same abandonment, already in the tree:
features/content_blocks/forms.feature— every scenario commented out under the header# Forms a brokenspec/inputs/name_input_spec.rb— three examples skipped with "Form addressibility removed 6 years ago"Changes
Deleted (34 files)
Cms::Form,Cms::FormField,Cms::FormEntryFormsController,FormFieldsController,FormEntriesControllercms/forms/,cms/form_fields/,cms/form_entries/cms/form_builder.js,cms/default-forms.scssfeatures/content_blocks/forms.featureModified — the removal itself (9 files)
config/routes.rb— 22 lines removed, including theallow_guests_to [:submit]endpoint, which was the largest unauthenticated surface in the engineapp/assets/javascripts/cms/application.js— dropped theform_builderrequirelib/cms/engine.rb,test/dummy/config/application.rb— droppedform_builder_csstest/factories/factories.rb— dropped the:formfactorytest/functional/cms/error_branches_test.rb— droppedFormFieldUpdateFailureBranchTestfeatures/step_definitions/content_pages_steps.rb— dropped 10 orphaned stepstest/unit/belongs_to_optionality_test.rb— dropped theCms::FormEntry#formandCms::FormField#formaudit entries; count tripwire 29 → 27spec/inputs/name_input_spec.rb— kept and repointed.NameInputis still used by every addressable type; Forms was only its fixture. Now tested againstDummy::Product, which is genuinely addressable. Un-skips three examples.Modified — upgrade-plan accuracy (4 files)
The removal invalidates live checklist items in the Rails-upgrade plan. Leaving them would send someone hunting for code that no longer exists.
docs/rails-upgrade/phase-5-the-5.0-bump.md— two live items. §5.5's manual verification of Forms is struck: there is nothing left to verify, so this removes a manual item rather than deferring one. §5.5a's "file tickets for the ten characterized defects" becomes seven — public form submission 500s, the Forms admin UI 500s and form_fields_controller#update` cannot fail are all closed by deletion. Of the two the plan named worst, only optimistic locking survives.docs/rails-upgrade/phase-4-report.md— annotated, not rewritten. It is the record of what Phase 4 found, so the three §3 rows keep their original wording and gain ✅ resolved by CMS-434; likewise §7's rationale row, §8's ticket item and §9's note thatform_entries_controllersits on Phase 5's manual list.RAILS_UPGRADE_TEST_PRIORITY.md— five passages. This is Phase 5 getting easier: B9's "four uncovered.deletesites are the entire job here" is now two, and both survivors are the security-relevant ones (strip_visibility_params,group_ids). The §7 controller-adequacy row cited four 0% files; three are gone. Therender text:row drops to one production site. ➕A4's uncovered.deliver!still needs converting but is no longer reachable from form submission..github/workflows/ci.yml— comments only. Thenext-railsjob listed both Forms defects as current findings and quoted per-suite counts this branch changed.Added — test output (2 files, commit
86d5f21d)Both came out of verifying this PR and are unrelated to Forms.
script/test-summary— runs the suite and prints only the results. It exists becauserake testis genuinely easy to misread: it runs five suites in separate processes and only the last one's summary reaches the end of the output, so a trailing0 failuressays nothing about the other four. Piping it is worse —rake test | tailreportstail's exit code, so an aborted run looks green. Both of those misread me during this work. The script prints every group's summary and exits with rake's own status.test/quiet_warnings.rb+ one line in.simplecov— filters gem-origin Ruby deprecation warnings, ~650 lines a run that minitest re-enables (Warning[:deprecated] = true) and that nobody can act on before the Rails 5 hop. Nothing that carries upgrade signal is touched: RailsActiveSupport::Deprecationoutput, minitest deprecations, and warnings from our own code all still print — there is one of the last kind today,sections_test.rb:179using::Fixnum, and it must stay visible because Ruby 3.2 removes it. Off underVERBOSE_WARNINGS=1orRUBYOPT='-W:deprecated'; CI already sets the latter, so its logs are unchanged.Deliberately not in this PR
cms_forms*tables remain, along with the migration that created them. They are empty in production, so there is no urgency, and dropping them is a separate destructive migration and a separate decision.skip_types = ['Cms::Form', ...]in CMSconnectors/new.html.erbis now a no-op. It compares strings, so it cannot raise — cleanup for a follow-up in that repo.lib/cms/version.rbstill readsCms::VERSION = '5.2.0', whichbrowsercms.gemspec:18publishes. rse/cms pins that exactly (Gemfile:11,Gemfile.lock:45), so releasing this branch to gem.fury as 5.2.0 would put two different builds behind one version number, and the app would not pick the change up anyway. Whoever cuts the release needs to bump it first.Worth noting the bump is about that collision, not about compatibility: this removes public API, but rse/cms is the only consumer we found and it does not use Forms -- which is the premise of this PR.
Testing
Gem suite
1. Check out the branch and install dependencies.
cd browsercms git checkout feature/cms-434-forms-subsystem bundle install2. Run the full suite. This branch adds a wrapper that does steps 2–4 in one go and handles the exit-code and per-group traps described below:
script/test-summary # full output lands in tmp/test-run.logExpect it to end in
PASS (rake exit 0).Or run rake directly
Do not pipe it —
rakeruns several test processes and a pipeline reports the exit code of the last command in the pipe, not rake's, so an aborted run looks green:Then confirm it did not abort. A trailing
0 failuresis not sufficient: only the last process's summary prints at the end, so an earlier group can fail while the tail looks clean.3. Check the per-group results.
script/test-summaryprints these directly.4. Confirm the warning filter is doing only what it claims. Two checks, since it is new in this PR:
5. Confirm no dangling references remain.
Expect three hits, all comments in two files:
spec/inputs/name_input_spec.rbexplaining why that spec was repointed atDummy::Product.test/dummy/db/schema.rbstill declares the tables, which is correct — they are not being dropped.test/unit/belongs_to_optionality_test.rbexplaining how that characterization test changed as a result of this PR.Host app smoke test
6. Point CMS at the local checkout. In its
Gemfile, lines 11–12:7. Install and boot the admin site.
Do not commit the resulting
Gemfile.lockchurn.8. From the admin, open the Assets menu. It iterates content types, so any stale
Cms::Formreference surfaces here immediately. Expect the same 29 types as before, with no Forms entry and no error.9. Open the New button dropdown (top right). Same iteration, different code path —
create_new: trueinlayouts/cms/_content_types.html.erb.10. Open the "Reuse Content" page — from a page, add content to a container and choose the reuse existing flow, not "Create New Content". The control to look at is the "Choose Type of Content"
<select>, which is the only one of these built fromCms::ContentType.connectablerather than from the hardcoded allowlist.layouts/cms/_content_types.html.erb, and show the same 29 entries. This step is the only one that exercises a different list.Expect the dropdown to be unchanged from before this branch.
Cms::Formwas connectable, so it was inconnectableandskip_typesfiltered it out of the display; now it is absent fromconnectableandskip_typesmatches nothing. Same visible result, reached differently — which is why the leftover'Cms::Form'string is harmless and is listedunder "Deliberately not in this PR" rather than fixed here.
11. Confirm the removed URLs no longer route. All five are under
/cms, the engine mount, so these are admin-side URLs. Routing resolves before any controller orbefore_actionruns, so a removed route 404s regardless of whether you are logged in — but see the exception below. Each should now 404, not 500, and not render a Forms page:The fifth is the one that matters, and it is not admin-only despite the URL.
POST /cms/form_entries/submitcarriedallow_guests_to [:submit], so it was reachable by anyone, unauthenticated — the largest thing this PR removes from the attack surface. It is a POST, so the address bar will not reach it. Test it logged out, which is how it was reachable:curl -i -X POST https://localhost:3000/cms/form_entries/submit \ -d "form_id=1&form_entry[email]=test@example.com"Confirmed 404 on a local run of this branch. Before it, the same request reached a controller that saved a record and then 500'd.
12. Load a Cognito form page — on the public site, not the admin. These are ordinary public pages: no
/cmsprefix, and the Guest group has view access, so no login is required. You'll need to restart the server without theENABLE_ADMINflag.With the app running, type the URL directly:
What you are checking: the page renders, and its source contains the embed —
13. Confirm assets compile.
application.jslost arequireanddefault-forms.scssis gone; nothing should reference either.bundle exec rake assets:precompile14. Revert the Gemfile swap and restore
Gemfile.lockin CMS.Before merging
15. Re-run the emptiness check. This is the entire basis for the PR, and it is cheap to repeat immediately before merge in case something wrote to these tables in the interim.
If either count is non-zero, stop and reassess — that data would need exporting first.