Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add is_frontedit GET parameter #1666

Draft
wants to merge 1 commit into
base: 6.dev
Choose a base branch
from

Conversation

litzinger
Copy link
Contributor

@litzinger litzinger commented Dec 16, 2021

Overview

Let add-on developers easily distinquish what is a Frontedit request and not a Frontedit request to change behaviors. Currently the only way to accurately test for this is something like this, which is a bit excessive. Having a single value to check would be easier, and also passing that value through the cp_js_end hook to those Ajax requests know their origin.

public function isFrontEditRequest(): bool
    {
        return !empty(ee()->input->get('entry_ids')) && ee()->input->get('modal_form') === 'y' && ee()->input->get('field_id') !== '';
    }

This change also requires a small update in the FrontEdit.php service file when creating the url:

$fieldEditUrl = ee('CP/URL')->make(
    'publish/edit/entry/ENTRY_ID',
    [
        'entry_ids' => ['ENTRY_ID'],
        'field_id' => 'FIELD_ID',
        'hide_closer' => 'y',
        'site_id' => 'SITE_ID',
        'modal_form' => 'y',
        'is_frontedit' => 'y',
        'return' => urlencode(ee()->functions->fetch_current_uri())
    ],
    ee()->config->item('cp_url')
);

I can't seem to make this change though because the Pro module is not in Github :(

Nature of This Change

  • πŸ› Fixes a bug
  • πŸš€ Implements a new feature
  • πŸ› Refactors existing code
  • πŸ’… Fixes coding style
  • βœ… Adds tests
  • πŸ‘½ Adds new dependency
  • πŸ”₯ Removes unused files / code
  • πŸ”’ Improves security

Is this backwards compatible?

  • Yes
  • No

Documentation

Not sure this requires a documentation update? Though I would consider adding something to the cp_js_end hook informing developers if they're using this hook to examine what they're doing because the Frontedit requests are not full control panel loads, so if they are adding JS to the page to modify something that isn't in the Frontedit modal, they could be adding excessive and unnecessary JS to the page load.

Let add-on developers easily distinquish what is a Frontedit request and not a Frontedit request to change behaviors.
@litzinger
Copy link
Contributor Author

litzinger commented Dec 16, 2021

Examples/use cases of when a developer can avoid excessive code and/or scripts from being included on the front-end than are needed.

    {
        $scripts = [];

        // If another extension shares the same hook
        if (ee()->extensions->last_call !== false) {
            $scripts[] = ee()->extensions->last_call;
        }

        // Don't load unnecessary files when it's a frontedit modal.
        if (ee()->input->get('is_frontedit') === 'y') {
            return implode('', $scripts);
        }

        if ($this->setting->get('developer_menu')) {
            ee()->load->library('session');
            $groups = $this->setting->get('show_publisher_menu');
            $groupId = App::userData('group_id');

            $menuController = new CustomMenuController($this->setting);
            $menuController
                ->setGroupId($groupId)
                ->setGroups($groups);

            $modules[] = 'var publisherAppendableMenuItems = ' . json_encode($menuController->getAppendableItems()) . ';';
            $modules[] = file_get_contents(PATH_THIRD_THEMES . 'publisher/scripts/publisher.menu.js');
        }

        $modules[] = file_get_contents(PATH_THIRD_THEMES .'publisher/scripts/publisher.edit.js');
        $modules[] = file_get_contents(PATH_THIRD_THEMES .'publisher/scripts/publisher.cp.js');
        $modules[] = file_get_contents(PATH_THIRD_THEMES .'publisher/scripts/jquery.autosize-min.js');
        $modules[] = $this->validateLicense();
        $modules[] = $this->versionCheck();

        return implode('', $scripts) . implode('', $modules);
    }

From another project...

    public function cp_js_end()
    {
        $scripts = [];

        // If another extension shares the same hook
        if (ee()->extensions->last_call !== false) {
            $scripts[] = ee()->extensions->last_call;
        }

        if (ee()->input->get('is_frontedit') === 'y') {
            return implode('', $scripts);
        }

        // Don't load these extra scripts (and a ton of extra stuff not included), which can be quite heavy
        $scripts[] = file_get_contents(FCPATH .'js/jira-issue-collector.js');
        $scripts[] = file_get_contents(FCPATH .'js/admin/custom-site-admin.js');

       // ... A lot more code here that only applies to the actual CP ...

@intoeetive
Copy link
Contributor

@litzinger sorry it took me so long to get back to you on this.

I do not like the idea of adding an extra $_GET variable. The check can be performed using this code instead

if (ee()->input->get('hide_closer') === 'y' && ee()->input->get('modal_form') === 'y') {
    // this is Pro front-end edit request
}

I am also not sure I understand the particular case of adding GET variable to cp_js_end call. If a certain add-on JS does not need to be executed in case of front-end edit request, it should simply be not loaded

Tagging @TomJaeger for any additional thoughts

@litzinger
Copy link
Contributor Author

litzinger commented Jan 18, 2022

The cp_js_end hook is a separate call entirely, so it has no idea what the context is. Only the calling script/request has access to hide_closer and modal_form, so it's not as simple as "If a certain add-on JS does not need to be executed in case of front-end edit request, it should simply be not loaded" because a normal front-end request is not the same as a front edit request... it's loading a lot of CP related things on the front-end.

I'm not sure why adding a new variable is not a good idea. The only way to figure out how to do this (as with many other things in EE that isn't documented) is to inspect everything about the request and code behind the feature. So you're expecting someone to figure out what hide_closer and modal_form are related to. Those words alone don't indicate any sort of active feature. modal_form or hide_closer could easily be re-used somewhere else on an entirely different feature in the future, then there would be nothing to indicate uniqueness. We already have the REQ constant, which could be PAGE, CP, or ACT... all very unique actions, but Front-edit is also very unique, but happens on the front-end, and also calls some CP related stuff. TBH I'd prefer to see a REQ == 'FRONTEDIT' option or something.

Please re-consider this request.

@intoeetive intoeetive marked this pull request as draft May 4, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants