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

Move insert tags for news, events etc. to the respective bundle #318

Closed
aschempp opened this issue Jul 28, 2015 · 11 comments

Comments

@aschempp
Copy link
Contributor

commented Jul 28, 2015

The InsertTags class in contao/core-bundle still handles inserttags for some of the other bundles. This should be refactored.

Feel free to assign this to me :-)

@leofeyer leofeyer added the feature label Aug 6, 2015

@leofeyer leofeyer added this to the 4.1.0 milestone Aug 6, 2015

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2015

This can only be correctly implemented after the hooks are converted to events…

@leofeyer

This comment has been minimized.

Copy link
Member

commented Aug 31, 2015

You know that is not true :)

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2015

Well I'm not gonna write a legacy class and a globals variable if we can add a correct event listener. Might depend on @Toflar too cause he wanted to suggest a new insert tags approach (ESI stuff).

@discordier

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2015

I would prefer to entirely render all of these as ESI via sub requests as discussed in Lyss.
Of course each of these routes could then be defined/handled in the corresponding bundle.

However, this is a breaking change in either implementation as currently you are only required to require contao/core-bundle to have the inserttag being replaced (as long as there are classes present with the correct names, NewsModel etc.). In future you will also need to require the news-bundle to have the insert tags in addition to the classes.
Granted, this is more of a bc breaking bugfix as the insert tags do not make much sense without the corresponding classes but I simply wanted to point this out. :)

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2015

I thought about the BC problem, but I think it’s not really valid. As you already pointed out, the insert tags will not work anyways in the current implementation (if e.g. contao/news-bundle is not installed).

@Toflar

This comment has been minimized.

Copy link
Member

commented Sep 1, 2015

Well, what would those InsertTags do if there's no e.g. news data? :D The BC break is that it would output {{news::foobar}} instead of replacing it with '' but I mean that's really neglectable.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2015

The BC break is that it would output {{news::foobar}} instead of replacing it with '' but I mean that's really neglectable.

Not true, insert tags are replaced with empty content (and logged) if there is no replacement.

@Toflar

This comment has been minimized.

Copy link
Member

commented Sep 1, 2015

Ah okay, so not even a BC break there...

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2015

I even guess it would be a bug fix because you would currently get exceptions (using unavailable classes and tables). But anyway, we need the events / ESI first, and I don't know if that will make it into 4.1…?

@discordier

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2015

Go @Toflar, go! ;)

@leofeyer leofeyer modified the milestones: 4.x.x, 4.1.0 Oct 6, 2015

@aschempp aschempp referenced this issue Oct 27, 2015
0 of 2 tasks complete
@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2016

see #471

@leofeyer leofeyer closed this Apr 8, 2016

@leofeyer leofeyer modified the milestones: 4.2.0, 4.x.x Apr 8, 2016

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