search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 822 forks source link

LeftAndMain/CMSMain/CMSPagesController should always identify the current page via URI path #6675

Closed Zauberfisch closed 7 years ago

Zauberfisch commented 7 years ago

Currently LeftAndMain->currentPageID() currently checks $this->getRequest()->requestVar('ID') for an ID of the current page.

When first opening a page everything is the way it should be: /admin/pages/edit/show/2, but SilverStripe Forms and FormFields are all request handlers that potentially could have have one more more nested sub request handlers. But once we want to access one of those request handlers (eg GridField, HTMLEditorField, UploadField) the path will become /admin/pages/edit/EditForm/field/MyGridField/item/new (this example is the resulting path for adding a new record in a GridField). Note the absence of the current page ID. At this time, the current page is actually saved into session or in some situation also take from hidden form inputs.

This issue in LeftAndMain - presumably a relic from older SilverStripe versions (new features such as the GridField handle this correctly for their children) - brings several problems that are completely avoidable:

  1. Links to sub request handlers are not shareable
  2. If you have the CMS open twice, you will have a broken context, because there can only be 1 current page stored in Session
  3. Nested RequestHandlers potentially break or cause unexpected side effects (such as HTMLEditorField - see related issue)
Zauberfisch commented 7 years ago

Also: any chance we can get a fix - should one be created by then - into the ss 4.0 release?

I feel like this is a rather significant change, as some modules or form fields might have worked around that in the past in hackish ways (such as HTMLEditorField_Toolbar), so it would make sense to have module developers update (if needed) to this new way now where they have to update anyway to support 4.

sminnee commented 7 years ago

This has been fixed in SS4 — do you want check out alpha7 which is being released right now?

Zauberfisch commented 7 years ago

I haven't tested it yet, but from looking at the code it does indeed look like it has been fixed.

Or at least almost. Because the checking of ?ID= is still inside LeftAndMain->currentPageID().

Do we have a specific reason why we are keeping that check in place? Is that perhaps still used when a form is submitted or something?

chillu commented 7 years ago

Note that the "current page" state of GridFields in SiteTree->getCMSFields() has been fixed in 3.6 through https://github.com/silverstripe/silverstripe-cms/commit/487235f9918b762f9eadcaaa655602da884807a6

Yes, the $this->getRequest()->requestVar('ID') is needed for form submissions.

Zauberfisch commented 7 years ago

As far as I see, form submission would now also include the ID in the URL, therefore requestVar() is not used by core at all anymore. So I assumed we are keeping it in for legacy reasons. But would very much appreciate removing it.

chillu commented 7 years ago

Yeah it would be nice to avoid the duplicate means to identify a record. I don't see how they'd get out of sync at the moment, so it's a low priority issue in my mind.