90/edit preview mode support v3 review

[mhsdesign]

TASK: Remove half-baked solution of session independent preview's
Otherwise, this feature would only work for this one page, successive links will currently not have that query param added - the linking service still needs to learn this, but we delay this for another time.

TASK: RenderingModeService instances runtime cache

TASK: Introduce constant RenderingMode::FRONTEND for system integrated rendering mode

[mficzel]

Like this in general. But the runtime cache implemented in the return statement is a bit too much flexing ;-)

My brain likes simple code and runtime caches that use early returns.

[mficzel]

I kinda dislike that you put the runtime cache in instances into one live with code that populates this.

Needlessly harder to read and unterstand. Offcourse the runtime cache makes sense.

[mficzel]

suggestion:

	public function findByName(string $modeName): RenderingMode
    {
        if ($instance = $this->instances[$modeName]) {
            return $instance;
        }
        if ($modeName === RenderingMode::FRONTEND) {
            $this->instances[$modeName] = RenderingMode::createFrontend();
        } else {
            $this->instances[$modeName] = RenderingMode::createFromConfiguration($modeName, $this->editPreviewModes[$modeName]);
        }
        return $this->instances[$modeName];        
    }

[mficzel]

??= and match($modeName) would be fine for me. What is imho too much is the match true you have to use to thwrow the exception. Will adjust and the merge this into the other pr.

Funny is that even the php documentation does not explain that operator other than mentioning its existence.

[mficzel]

BTW: This would be fine for me ... but it will not throw on unconfigured modes

return $this->instances[$modeName] ??= match($modeName) {
   RenderingMode::FRONTEND => RenderingMode::createFrontend(),
   default => RenderingMode::createFromConfiguration($modeName, $this->editPreviewModes[$modeName])
};

[mhsdesign]

Like this in general. But the runtime cache implemented in the return statement is a bit too much flexing ;-)

hehe i do understand what you mean. I was inspired by code I saw written from @grebaldi

phps newer match and ??= are indeed very powerful features that allow to write code much more compact. (On the cost of readability for people unfamiliar with this pattern)

i think at one point we might get used to such patterns more and that this is for now too early to use?

[mhsdesign]

I won’t be on my pc today so please feel free to adjust the code as you liked (this was just a suggestion and it’s your pr after all ^^)