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

Fix the SERP preview #1092

Merged
merged 7 commits into from Dec 20, 2019
Merged

Fix the SERP preview #1092

merged 7 commits into from Dec 20, 2019

Conversation

@leofeyer
Copy link
Member

leofeyer commented Dec 12, 2019

Since #831 has not made it into Contao 4.9, we need a different solution to retrieve the URL from the model. This PR does the following:

  • Support callables as URL ('serpPreview' => array('url' => function () {}))
  • Only add the input field suffix if the field name is not empty
@leofeyer leofeyer added the defect label Dec 12, 2019
@leofeyer leofeyer added this to the 4.9 milestone Dec 12, 2019
@leofeyer leofeyer requested review from ausi, Toflar, bytehead and aschempp Dec 12, 2019
@leofeyer leofeyer self-assigned this Dec 12, 2019
Copy link
Contributor

aschempp left a comment

Can you explain why contao.url_suffix is necessary? Shouldn't we use the second explode() result instead? Also because the placeholder does not necessarily need to be at the end of the URL (before the suffix).

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 13, 2019

Shouldn't we use the second explode() result instead?

You are absolutely right. Changed in 584e0e8.

@leofeyer leofeyer requested a review from aschempp Dec 13, 2019
@leofeyer leofeyer requested a review from aschempp Dec 16, 2019
Copy link
Contributor

aschempp left a comment

I only have one problem with this now: If I want to use a service with tags or annotations to set up the callback, I can't use it, because Contao would automatically append _callback to the url attribute. Because that's what all callbacks are built like.

Maybe it would be better to call it url_callback instead? I also wonder why the serpPreview settings are grouped in the eval, we never do that anywhere (like the files, filesOnly etc. attributes for FilePicker are not grouped either).

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 16, 2019

I can't use it, because Contao would automatically append _callback to the url attribute

What exactly do you mean?

I also wonder why the serpPreview settings are grouped in the eval, we never do that anywhere

Yes, we do: 'dcaPicker' => array()

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Dec 16, 2019

I can't use it, because Contao would automatically append _callback to the url attribute

What exactly do you mean?

https://github.com/contao/contao/blob/master/core-bundle/src/DependencyInjection/Compiler/DataContainerCallbackPass.php#L83

I also wonder why the serpPreview settings are grouped in the eval, we never do that anywhere

Yes, we do: 'dcaPicker' => array()

Well that's different to me, these are settings for the DC that are unrelated to the widget itself (they're for a wizard in that case).

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 18, 2019

I can't use it, because Contao would automatically append _callback to the url attribute

You cannot use the @Callback annotation, because this is not a DCA callback such as the load_callback or the save_callback. It just a config value which allows to pass a callable.

I also wonder why the serpPreview settings are grouped in the eval, we never do that anywhere

Changed in 4ef0579.

@leofeyer leofeyer requested a review from aschempp Dec 18, 2019
Copy link
Contributor

aschempp left a comment

You cannot use the @callback annotation, because this is not a DCA callback such as the load_callback or the save_callback. It just a config value which allows to pass a callable.

It is exactly the same, why should I not be able to use it? The annotation target is fields.serpWidget.eval.url, but the tag listener would convert that to fields.serpWidget.eval.url_callback 😉

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 18, 2019

It is exactly the same, why should I not be able to use it?

This is more or less the same argument as

I also wonder why the serpPreview settings are grouped in the eval, we never do that anywhere

We also never define callbacks in the eval section. 🤷‍♂ To me it is just a parameter that happens to support callables, which however does not make it a DCA callback.

Also, please keep in mind that this is just an interim solution until we can pass the model to the router.

@leofeyer leofeyer requested a review from aschempp Dec 19, 2019
throw new \RuntimeException(sprintf('Unsupported model class "%s"', \get_class($model)));
if (\is_array($this->url_callback))
{
$this->import($this->url_callback[0]);

This comment has been minimized.

Copy link
@aschempp

aschempp Dec 19, 2019

Contributor

I would much prefer System::importStatic()

This comment has been minimized.

Copy link
@leofeyer

leofeyer Dec 20, 2019

Author Member

Why? There is absolutely no difference between the two, except that importStatic() can be used in a non-object context.

This comment has been minimized.

Copy link
@aschempp

aschempp Dec 20, 2019

Contributor

and import() creates a local variable in the magic getter method, and the imported object is from now on bound to the object, whereas it is released automatically after the method call otherwise.

This comment has been minimized.

Copy link
@leofeyer

leofeyer Dec 20, 2019

Author Member

Valid point. Changed in 7d55514.

@leofeyer leofeyer requested a review from aschempp Dec 20, 2019
@leofeyer leofeyer requested a review from aschempp Dec 20, 2019
@leofeyer leofeyer merged commit 32e7d52 into master Dec 20, 2019
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 89.29% (+0.19%) compared to ab3da98
Details
@leofeyer leofeyer deleted the bugfix/serp-preview branch Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.