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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the insert tags in picker providers configurable #450

Merged
merged 10 commits into from Jul 5, 2019

Conversation

@Toflar
Copy link
Member

commented Apr 24, 2019

I just had the problem that I wanted to use the page picker but not with the default {{link_url::<value>}} insert tag because I need absolute links, so I want the |absolute flag to always be added.
I noticed that the insert tag is unfortunately hard coded which I find a very limiting factor for a general picker provider.
So here we go, you can now specify whatever insert tag you like by specifying

'eval' => [
    'dcaPicker' => [
        'insertTag' => '{{my_super_insert_tag::%s}}',
    ],
],

which will take the insert tag for all picker providers (news, page etc.), though.
However, you can also do this:

'eval' => [
    'dcaPicker' => [
        'pagePicker' => [
            'insertTag' => '{{link_url::%s}}',
        ],
        'newsPicker' => [
            'insertTag' => '{{news_url::%s}}',
        ],
    ],
],

I've implemented it for all the core pickers so it will also work for news, faq, events etc. not just for the page picker provider 馃槉
Also there's a new $config->getExtraForProvider() so pickers can implement fetching provider specific extras falling back to general extras if they want to.

@Toflar Toflar force-pushed the Toflar:picker-configurable-inserttag branch 2 times, most recently from 136b258 to 1bafbe6 Apr 24, 2019

@Toflar Toflar force-pushed the Toflar:picker-configurable-inserttag branch from 1bafbe6 to 9ee3835 Apr 24, 2019

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

I just noticed an issue here. I need to be able to specify the extras to be only applicable for e.g. the page picker but not the news picker.
How do I do that? @contao/developers

@leofeyer leofeyer closed this Apr 25, 2019

@leofeyer leofeyer reopened this Apr 25, 2019

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

Updated PR to support extras per provider and also updated the issue description to demonstrate the usage 馃槃

@leofeyer leofeyer added the feature label Apr 30, 2019

@leofeyer leofeyer added this to the 4.8.0 milestone Apr 30, 2019

@@ -45,7 +47,9 @@ public function supportsContext($context): bool
*/
public function supportsValue(PickerConfig $config): bool
{
return false !== strpos($config->getValue(), '{{event_url::');
$insertTagChunks = explode('%s', $this->getInsertTag($config, self::DEFAULT_INSERTTAG), 2);

This comment has been minimized.

Copy link
@leofeyer

leofeyer Apr 30, 2019

Member

Can we encapsulate this in a getInsertTagChunk() method so we do not have to use multiple explode() calls?

This comment has been minimized.

Copy link
@Toflar

Toflar May 1, 2019

Author Member

I'm not even sure if that should be named insertTag or if that should be something else? I mean, in fact it's just whatever string you want to provide and we're going to replace '%s' with the value. I just don't know if there's any other use case other than an insert tag 馃槃 wdyt?

This comment has been minimized.

Copy link
@leofeyer

leofeyer May 2, 2019

Member

I just don't know if there's any other use case other than an insert tag

I cannot think of one.

This comment has been minimized.

Copy link
@Toflar

Toflar May 2, 2019

Author Member

Done 馃槃

@Toflar Toflar force-pushed the Toflar:picker-configurable-inserttag branch from dc5d0b4 to 51f997d May 2, 2019

@aschempp

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Instead of splitting the default inserttag by %s, why not simply store the prefix + suffix in the class constant? Or even more so have a _PREFIX and _SUFFIX?

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

Because it makes no sense to me. %s as the placeholder for sprintf() is well known so I don't really want to configure a value prefix and suffix in the dca configuration.

@aschempp

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

I don't really want to configure a value prefix and suffix in the dca configuration.

Yeah, I would configure an array with two values for that case. I don't see why we would need to split on every use instead of already having an array.

* Provides a shortcut get the "insertTag" extra value for child classes and
* split them at the placeholder (%s).
*/
protected function getInsertTagChunks(PickerConfig $config, string $default): array

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jun 5, 2019

Member

Can we not set a default value for the second argument?

string $default = static::DEFAULT_INSERTTAG

Due to the late static binding, this should be set to the value of the constant of the child class.

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 5, 2019

Author Member

And what if I forget to set this in my child? I would only notice it at runtime which makes the whole idea of an abstract class and interfaces pointless 馃槃

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jun 5, 2019

Member

It would use the constant of the parent class then.

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 5, 2019

Author Member

Which does not exist because what value would it provide? 馃槃

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jun 5, 2019

Member

Ok, then why did you make the constant protected instead of private? Also, the naming is misleading: DEFAULT_INSERTTAG implies that there are constants for other, non-default insert tags, which there are not. We should change it to just INSERTTAG.

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 6, 2019

Author Member

So that they can be replaced by a class extending a specific picker? I don't think the name is misleading, it's the default insert tag if no other configuration was provided.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I just noticed an issue here. I need to be able to specify the extras to be only applicable for e.g. the page picker but not the news picker.

Is this still an unsolved problem?

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Is this still an unsolved problem?

Bildschirmfoto 2019-06-05 um 13 45 37

@leofeyer leofeyer force-pushed the contao:master branch 2 times, most recently from 6c52109 to 03f6899 Jun 5, 2019

@leofeyer leofeyer force-pushed the contao:master branch from 67bdc5c to d42ccf4 Jun 13, 2019

@leofeyer
Copy link
Member

left a comment

Besides what I have already commented, there are still some major issues with the implementation that I do not like.

  1. The whole "insert tag chunks" logic is quite cumbersome IMHO. It turns a readable '{{event_url::' into $this->getInsertTagChunks($config, self::DEFAULT_INSERTTAG)[0].

  2. I find it completely wrong that we have to pass the insert tag constant in the getInsertTag() method just so the abstract parent class knows about the insert tag. If the parent class could access the constant (or call a method) of the child class, we could simplify getInsertTag($config, self::$DEFAULT_INSERTTAG) to getInsertTag($config). The same applies to the getInsertTagChunks() method.

  3. The constant DEFAULT_INSERTTAG is misleading, because it suggests that there are other, non-default constants. I suggest renaming it to INSERTTAG.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

  1. he whole "insert tag chunks" logic is quite cumbersome IMHO. It turns a readable '{{event_url::' into $this->getInsertTagChunks($config, self::DEFAULT_INSERTTAG)[0].

Yeah well, it's the whole point of this PR though. We want to make something that is hardcoded dynamic so I'm sorry but I cannot really change this fact.

  1. I find it completely wrong that we have to pass the insert tag constant in the getInsertTag() method just so the abstract parent class knows about the insert tag. If the parent class could access the constant (or call a method) of the child class, we could simplify getInsertTag($config, self::$DEFAULT_INSERTTAG) to getInsertTag($config). The same applies to the getInsertTagChunks() method.

I agree with that. I've reworked the concept in 418c264. Do you like that better now? Of course, I cannot make the getFallbackInsertTag() method abstract because that would mean every picker that extends the AbstractPickerProvider would need to implement. This would be a) a BC break and b) it's not necessary anyway as you only have to provide it if you want to work with insert tags.

  1. The constant DEFAULT_INSERTTAG is misleading, because it suggests that there are other, non-default constants. I suggest renaming it to INSERTTAG.

I've gotten rid of the constants completely and I also moved to the term "fallback insert tag" rather than "default insert tag".

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

What was wrong with the constants?

class AbstractPickerProvider
{
    public const INSERTTAG = null;

    public function getInsertTag(): string
    {
        if (null === static::INSERTTAG) {
            throw new \LogicException('Please add a public INSERTTAG constant in your picker provider class');
        }
        
        return static::INSERTTAG;
    }
}

class PagePickerProvider extends A
{
    public const INSERTTAG = '{{page::%s}}';
}

See https://3v4l.org/pRVpe

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

Adjusted accoringly. I made them protected though because they don't need to be accessed from the outside.

@leofeyer leofeyer force-pushed the Toflar:picker-configurable-inserttag branch from 97f9c87 to c16f3f1 Jul 5, 2019

@Toflar

This comment has been minimized.

Copy link
Member

commented on core-bundle/src/Picker/AbstractPickerProvider.php in 5db3ad7 Jul 5, 2019

I don't like the naming. It should be getInsertTagValue() or so. This is an abstract class, you can also extend it without working with insert tags in which case getValue() sounds strange if it then does something with insert tags.

This comment has been minimized.

Copy link
Member Author

replied Jul 5, 2019

I initially had it named getInsertTagValue(). I'm going to change it again.

@leofeyer leofeyer force-pushed the Toflar:picker-configurable-inserttag branch from cd07346 to 053d43d Jul 5, 2019

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

@leofeyer thinking of it, maybe we should have an AbstractInsertTagsPickerProvider that extends the AbstractPickerProvider and provides the additional methods. Imho that would be cleaner design because then you don't get any insert tag related helper methods in autocompletion when you don't want to work with insert tags in your picker provider.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Is there a picker that does not use insert tags?

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Sure! Have a look at https://github.com/terminal42/contao-bynder/blob/master/src/Picker/BynderAssetPickerProvider.php for example :) This one is not even extending the AbstractPickerProvider :D

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Implemented in b82b5ad.

@leofeyer leofeyer force-pushed the Toflar:picker-configurable-inserttag branch from a5ec382 to b82b5ad Jul 5, 2019

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

I like that better, wdyt :)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

I like it, too. Gonna run one last test and then I would merge it if you want.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Go for it 馃憤 Thank you!

@leofeyer leofeyer merged commit f55800c into contao:master Jul 5, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Travis CI - Pull Request Build Canceled
Details
coverage/coveralls Coverage decreased (-0.002%) to 87.144%
Details
@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Thank you @Toflar.

@Toflar Toflar deleted the Toflar:picker-configurable-inserttag branch Jul 5, 2019

@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

Sorry, but this is a BC break as well as a bad idea. Removing protected methods from a class that is supposed to be extended is not semver. Also, we should never expect a class constant, that's what abstract methods are for!

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Nobody removed any methods?

@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

wtf, looks like I had an outdated view of the PR. The constant still remains though. Can we use an abstract method instead of the constant please? This way all IDEs will tell you to implement that method.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

I agree that would be better :)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

I'm going to change it.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Changed in 92f4a9e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.