dnadesign / silverstripe-elemental-userforms

Adds a new element for usersforms
BSD 3-Clause "New" or "Revised" License
9 stars 16 forks source link

FIX Check current controller has a functional link #61

Closed emteknetnz closed 2 years ago

emteknetnz commented 2 years ago

Issue https://github.com/silverstripe/silverstripe-fulltextsearch/issues/324

Have tested that no error is triggered during solr reindex

GuySartorelli commented 2 years ago

For anyone who hasn't seen the context, this is catching the trigger_error from RequestHandler:

https://github.com/silverstripe/silverstripe-framework/blob/cb37869bac40ffae2458654f842bcb173be61931/src/Control/RequestHandler.php#L575-L578

I suspect that's there because ElementForm could hypothetically be used in a variety of contexts that don't necessarily use a SiteTree (elemental blocks can belong to arbitrary DataObjects). There is an assumption here that the context will have a valid front-end link available from the controller, but it isn't necessarily safe to assume the controller will be a subclass of ContentController for example.

I think the correct fix for this is to say "get the link from my parent object" instead of "get the link from whatever the controller is currently active", which in most cases would result in the same link (I think), but in cases such as running some BuildTask or queued job would mean it fetches a correct front-end link here.

That said, I don't know if there might be some edge cases where that has unexpected results. @emteknetnz What are your thoughts on that approach? If you can foresee issues with that then I think this fix is okay, given that it isn't making anything more broken, and does fix the problem it's intended to fix.

emteknetnz commented 2 years ago

BaseElement has a method getPage() with the comment Despite the name of the method, getPage can return any type of DataObject

So we could go

$obj = $this->getPage();
$controller = ModelAsController::controller_for($obj); // << only works for SiteTree, substitute for DataObject equivalent
$link = $controller ? $controller->Link() : '';

Though what happens if the DataObject's controller (often won't even have a controller) lacks a configured url_segment - then we're back to the same problem?

GuySartorelli commented 2 years ago

We'd have to check if ClassInfo::method_exists(get_class($this->getPage(), 'Link') (or something like that) to see if there is even a Link() method on the parent data object class.

If there is no Link() on the class, then we can throw an exception because that means they're trying to use userforms in a dataobject that doesn't support them.

If there is a Link() method but it results in this user warning, I think we can just let that bubble up (or catch it and thrown an exception again) because, again, that means they're trying to use userforms where they shouldn't be.

GuySartorelli commented 2 years ago

We've discussed this a bit offline. For fixing this error, changing the logic for where the link comes from is out of scope. That should definitely be addressed, but it's too risky to be doing that in this PR.

GuySartorelli commented 2 years ago

Approve, but I don't have access to merge.