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

Allow element types to set additional query params used in getElementById() #3117

Closed
carlcs opened this issue Jul 19, 2018 · 13 comments
Closed

Comments

@carlcs
Copy link
Contributor

carlcs commented Jul 19, 2018

In my element type I have a query parameter which conceptually works very similar to status, where if it isn’t explicitly set it defaults to a specific value.

I’m now running into issues with the Elements::getElementById() function, which doesn’t always return my elements because I’d need to set/override my param to null for it to work. I am not calling getElementById() myself, it’s called by Elements::propagateElement() when I save my element.

It would be nice if it was possible to set additional query params used in getElementById(), maybe via a static function that I could add to my element class or via an event listener.

This is the code I have in the constructor of my ElementQuery:

// Default status
if (!isset($config['status'])) {
    $config['status'] = [Task::STATUS_INPROGRESS];
}

// Default environment
if (!isset($config['environment'])) {
    $config['environment'] = Plugin::getInstance()->getEnvironments()->getCurrentEnvironment();
}
@brandonkelly
Copy link
Member

Even if we did, you wouldn’t be able to override the code that calls getElementById() so your params would still be the same, right?

@carlcs
Copy link
Contributor Author

carlcs commented Jul 20, 2018

I don’t need to override that code that calls it. What I want to do is to override the default environment param in getElementById(). Similar to what you do with status, you set a default in ElementQuery and overrride it in getElementById().

@carlcs
Copy link
Contributor Author

carlcs commented Jul 20, 2018

A default status of “live” makes sense for entry queries, but you can’t have that set for getElementById().

In my use case a default environment of the current environment ID makes sense, but this causes issues with getElementById() if I can’t override it like you can.

@brandonkelly
Copy link
Member

brandonkelly commented Jul 20, 2018

OK, just so I’m clear, you are asking for a way to register custom query params that are only applied when getElementById() is called?

What if instead you just don’t set the environment param at all from your constructor, and determine the default value for it from your beforePrepare() method instead?

protected function beforePrepare(): bool
{
    // ...

    // Determine the environment
    $env = $this->environment ?? (empty($this->id)
        ? Plugin::getInstance()->getEnvironments()->getCurrentEnvironment()
        : null);

    // Now use $env instead of $this->environment ...
}

@carlcs
Copy link
Contributor Author

carlcs commented Jul 20, 2018

OK, just so I’m clear, you are asking for a way to register custom query params that are only applied when getElementById() is called?

Yes, exactly.

What if instead you just don’t set the environment param at all from your constructor, and determine the default value for it from your beforePrepare() method instead?

Well, it still must be possible to set the param to null for the places where I have to query for all elements. I could use false as a value for this, and I actually had that as a workaround, but I actually would prefer to be consistant with the status parameter values.

This is what I currently do to work around errors being thrown by Elements::propagateElement(). In beforePrepare():

if (
    !Craft::$app->getRequest()->getIsConsoleRequest() &&
    Craft::$app->getRequest()->getActionSegments() === ['maintenance', 'tasks', 'save-task']
) {
    $this->environmentId = null;
}

With both workaround solutions my element type isn’t really compatible to expected behavior ofgetElementById() though, it might not return an element even though it exists.

@brandonkelly
Copy link
Member

Maybe a better option would be for us to add a new method to ElementQuery, perhaps called nofilter(), which sets $this->status = null and $this->enabledForSite = false internally, and start calling that everywhere that we’re currently manually setting both of those params (such as getElementById()) instead.

Then you could just override that method, and null out your environment param.

public function nofilter()
{
    $this->environment = null;
    return parent::nofilter();
}

@carlcs
Copy link
Contributor Author

carlcs commented Jul 20, 2018

This would be perfect.

@carlcs
Copy link
Contributor Author

carlcs commented Jul 20, 2018

Or a 3rd argument for the constructor?

public function __construct($elementType, array $config = [], bool $nofilter = false);

@brandonkelly
Copy link
Member

Thought about that, but a lot of the places we currently null-out the status and enabledForSite params are places where that code wasn’t where the query got created in the first place.

@brandonkelly
Copy link
Member

All set now. We decided to call it anyStatus(), since that’s closer to what the method does in the base class, and nofilter would imply that other things like id, section, etc., should also be nulled out.

@carlcs
Copy link
Contributor Author

carlcs commented Jul 21, 2018

Thank you Brandon! When I said perfect I lied a little bit, I didn't like the name. With naming it anystatus it's perfect for real.

@brandonkelly
Copy link
Member

Hah, good to head! We had to debate it for a bit; the only issue we had with anyStatus is it didn’t seem like it was a good fit for your environment use case :)

@carlcs
Copy link
Contributor Author

carlcs commented Jul 25, 2018

Well I consider it a status-like property of my element. It is just another way to disable it under certain conditions, I think you can say it is defining its status.

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

No branches or pull requests

2 participants