NEW Move form schema logic into FormSchemaController #1850

GuySartorelli · 2024-11-12T03:34:57Z

In a perfect world LeftAndMain would not be a subclass of the new FormSchemaController class - but so many of its subclasses rely directly on that logic, and refactoring those to spin off sub-controllers looks like way more work than it's really worth.

I thought of making the new class a trait instead, but if a trait has private static properties the subclass can't also have those private static properties.
We could implement getExtraMethodConfig() in the trait to work around that, but that's an ugly solution imo.

This PR still takes us closer to making LeftAndMain abstract (since it doesn't need to route Modals anymore) and makes it easier to have non-LeftAndMain form schema controllers, and those are the main goals for this card.

Issue

Refactor formschema logic from LeftAndMain into its own abstract class #1762

GuySartorelli · 2024-11-12T03:35:39Z

code/FormSchemaController.php

+        if (!$this->schema) {
+            $this->schema = FormSchema::singleton();
+        }


This wasn't there in LeftAndMain but it's a little protection in case the dependency doesn't get injected (e.g. not using injector to instantiate this controller for some reason)

GuySartorelli · 2024-11-12T03:36:57Z

code/FormSchemaController.php

The code in this class is mostly directly copied in from LeftAndMain.

Note that there were no existing unit tests explicitly for the methods in this class - instead, this functionality is fairly heavily tested in behat (linkfield, elemental, history controller, WYSIWYG link modals all use it)

GuySartorelli · 2024-11-13T23:30:45Z

code/FormSchemaController.php

+        // The getter can accept an ID where the main form action wouldn't
+        $formMethod = "get{$formName}";
+        if (!$this->hasMethod($formMethod)) {
+            $formMethod = $formName;
+            if (!$this->hasMethod($formMethod)) {
+                $this->jsonError(404, 'Form not found');
+            }
+        }


Most usages of this method right now (e.g. see AssetAdmin) have two methods - one method is named the same as the form, and does not accept a parameter - the other prefixed with get and accepts a parameter.

The usages of ModalController used to go through methodSchema on LeftAndMain - but now we just go directly through schema on ModalController. Those methods do not have get prefixes.

I'm not going through and refactoring all of those - that's out of scope. Instead, I've just updated this method to account for both scenarios.

Could you put some the github comment info into the inline comment - by itself the existing comment "The getter can accept an ID where the main form action wouldn't" is pretty confusing

Happy to do so, but I'm not sure what you find confusing about it so I don't know what info you want in there. What would you like the comment to say? Or what about the existing comment is confusing?

Most usages of this method (e.g. AssetAdmin) have two methods - one method is named the same as the form, and does not accept a parameter - the other prefixed with "get" and accepts an $itemID parameter.

^ Maybe just the comment to this

GuySartorelli · 2024-11-13T23:51:12Z

code/LeftAndMain.php

-     * Get the form schema from a given method.
-     * The method must return a Form.
-     */
-    public function methodSchema(HTTPRequest $request): HTTPResponse


This method was just a bizarre way to avoid directly linking to ModalController.
It's not used anywhere now that we're linking to ModalController directly. If anyone's using it directly in their own projects or modules, the recommendation will be to directly link to the schema for the FormSchemaController that handles that form, instead of this indirection.

GuySartorelli · 2024-11-14T02:31:39Z

code/FormSchemaController.php

+    protected function init()
+    {
+        $this->beforeExtending('onInit', function () {
+            // Set the members html editor config
+            if (Security::getCurrentUser()) {
+                HTMLEditorConfig::set_active_identifier(Security::getCurrentUser()->getHtmlEditorConfigForCMS());
+            }
+
+            // Set default values in the config if missing.  These things can't be defined in the config
+            // file because insufficient information exists when that is being processed
+            $htmlEditorConfig = HTMLEditorConfig::get_active();
+            $htmlEditorConfig->setOption('language', TinyMCEConfig::get_tinymce_lang());
+            $langUrl = TinyMCEConfig::get_tinymce_lang_url();
+            if ($langUrl) {
+                $htmlEditorConfig->setOption('language_url', $langUrl);
+            }
+        });
+        parent::init();
+    }


Needed for controllers such as ElementalAreaController which may include WYSIWYG in their forms.

GuySartorelli · 2024-11-14T20:29:29Z

code/LeftAndMain.php

@@ -104,7 +94,6 @@ class LeftAndMain extends AdminController implements PermissionProvider
     *
     * @config
     * @var string
-     * @deprecated 2.4.0 Will be renamed to model_class


unrelated - was missed in a merge-up

emteknetnz · 2024-11-14T04:33:31Z

code/FormSchemaController.php

+    {
+        $this->beforeExtending('onInit', function () {
+            // Set the members html editor config
+            if (Security::getCurrentUser()) {


Looks kind of messy having all of this here around the top of the file since it's all TinyMCE related which is weird in a FormSchema class. Move it to a private method down the bottom of the class

TinyMCE is in forms, so form schema having form stuff doesn't seem weird to me. Done anyway though.

emteknetnz · 2024-11-14T04:34:04Z

code/FormSchemaController.php

+    }
+
+    /**
+     * Get form schema helper


Suggested change

* Get form schema helper

* Get the form schema helper for this controller

emteknetnz · 2024-11-14T04:34:12Z

code/FormSchemaController.php

+    }
+
+    /**
+     * Set form schema helper for this controller


Suggested change

* Set form schema helper for this controller

* Set the form schema helper for this controller

emteknetnz · 2024-11-14T04:36:52Z

code/FormSchemaController.php

+        }
+
+        if (!$this->hasAction($formName)) {
+            $this->jsonError(401, 'Form not accessible');


Suggested change

$this->jsonError(401, 'Form not accessible');

$this->jsonError(404, 'Form not accessible');

It's an invalid route, rather than an issue with a user not being logged in

Changing error codes is not in scope for this PR. This isn't new code.

emteknetnz · 2024-11-14T22:31:01Z

code/FormSchemaController.php

+        // The getter can accept an ID where the main form action wouldn't
+        $formMethod = "get{$formName}";
+        if (!$this->hasMethod($formMethod)) {
+            $formMethod = $formName;
+            if (!$this->hasMethod($formMethod)) {
+                $this->jsonError(404, 'Form not found');
+            }
+        }


Could you put some the github comment info into the inline comment - by itself the existing comment "The getter can accept an ID where the main form action wouldn't" is pretty confusing

GuySartorelli commented Nov 12, 2024

View reviewed changes

GuySartorelli marked this pull request as draft November 12, 2024 03:38

GuySartorelli force-pushed the pulls/3/formschema branch 2 times, most recently from 26138b8 to 0fb29a4 Compare November 13, 2024 23:22

GuySartorelli commented Nov 13, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/formschema branch from 0fb29a4 to e1e8534 Compare November 13, 2024 23:49

GuySartorelli commented Nov 13, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/formschema branch from e1e8534 to daba832 Compare November 14, 2024 02:30

GuySartorelli commented Nov 14, 2024

View reviewed changes

GuySartorelli mentioned this pull request Nov 14, 2024

API Deprecate API that will be removed as a result of refactoring #1851

Merged

GuySartorelli force-pushed the pulls/3/formschema branch from daba832 to 19b9eda Compare November 14, 2024 20:28

GuySartorelli commented Nov 14, 2024

View reviewed changes

GuySartorelli marked this pull request as ready for review November 14, 2024 21:01

emteknetnz requested changes Nov 14, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/formschema branch from 19b9eda to c0a917e Compare November 14, 2024 22:44

NEW Move form schema logic into FormSchemaController

4d72be8

GuySartorelli force-pushed the pulls/3/formschema branch from c0a917e to 4d72be8 Compare November 18, 2024 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Move form schema logic into FormSchemaController #1850

NEW Move form schema logic into FormSchemaController #1850

GuySartorelli commented Nov 12, 2024

GuySartorelli Nov 12, 2024

GuySartorelli Nov 12, 2024

GuySartorelli Nov 13, 2024

emteknetnz Nov 14, 2024

GuySartorelli Nov 14, 2024

emteknetnz Nov 15, 2024

GuySartorelli Nov 18, 2024

GuySartorelli Nov 13, 2024 •

edited

Loading

GuySartorelli Nov 14, 2024

GuySartorelli Nov 14, 2024

emteknetnz Nov 14, 2024

GuySartorelli Nov 14, 2024

emteknetnz Nov 14, 2024

GuySartorelli Nov 14, 2024

emteknetnz Nov 14, 2024

GuySartorelli Nov 14, 2024

emteknetnz Nov 14, 2024

GuySartorelli Nov 14, 2024

emteknetnz Nov 14, 2024

	* Get form schema helper
	* Get the form schema helper for this controller

	* Set form schema helper for this controller
	* Set the form schema helper for this controller

	$this->jsonError(401, 'Form not accessible');
	$this->jsonError(404, 'Form not accessible');

NEW Move form schema logic into FormSchemaController #1850

Are you sure you want to change the base?

NEW Move form schema logic into FormSchemaController #1850

Conversation

GuySartorelli commented Nov 12, 2024

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Nov 13, 2024 •

edited

Loading