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

Incorrect deprecation of info_block, causes B/C break, does not load template overrides #15805

Closed
weeblr opened this issue May 3, 2017 · 12 comments

Comments

@weeblr
Copy link
Contributor

weeblr commented May 3, 2017

Steps to reproduce the issue

Create a JLayout override for joomla.content.info_block.block in your template.

Expected result

The override is used instead of Joomla built-in JLayout.

Actual result

The override is not used. Instead Joomla now loads the newly added joomla.content.info_block

Additional comments

This is B/C break from previous Joomla version, due to an incorrect deprecation procedure. The joomla.content.info_block.block content was removed and put into joomla.content.info_block (why? - but that does not matter here). A deprecation notice was added but that's missing the point.

Joomla 3.7.0 now loads directly joomla.content.info_block, while potential existing layout overrides were made for joomla.content.info_block.block

This requires that all users with such overrides go and move and rename their override files to the new location.

The correct proceudre would be to deprecate, but still load the "old" JLayouts, and only load the new one when an API change is allowed.

@laoneo
Copy link
Member

laoneo commented May 5, 2017

True, here is the reference to the pr #12966 why this has changed.

@weeblr
Copy link
Contributor Author

weeblr commented May 5, 2017

Hi @laoneo

Yes, I can see why this was done, but that does not change the fact this is major B/C break.

As a side note, the main justification

The main benefit of this is that it makes it easier to override parts of a layout.

is incorrect, using render() or sublayout does not change anything to how easy you can override parts of layouts.

The point is that this actually broke any override made on existing sites for the "block" JLayout.

Rgds

@laoneo
Copy link
Member

laoneo commented May 5, 2017

Can you do a pr to be BC compatible?

@weeblr
Copy link
Contributor Author

weeblr commented May 5, 2017

Can't the PR just be reverted? It introduces changes on a wrong premises, with no added value?

Rgds

@laoneo
Copy link
Member

laoneo commented May 5, 2017

It would be then a BC break against Joomla 3.7.0. @rdeutz what do you think?

@weeblr
Copy link
Contributor Author

weeblr commented May 5, 2017

That's right. Meaning we also need a stub joomla.content.info_block.

@weeblr
Copy link
Contributor Author

weeblr commented May 5, 2017

Ah crap, no. Switching to $this->sublayout() is not equivalent to JLayoutHelper::render(), it introduces subtle changes, what with the data in the layout, include paths, etc being used in sublayout(), while they are not in render().... Big mess now.

Fixing is no more as easy as reverting and adding a stub. I'll try to think about something.

Maybe this can be left as is, but simply include the current joomla.content.info_block.block stub instead of including joomla.content.info_block.

I can do a PR for that, but I can't just do it from Github, as it needs testing, so it'll have to wait til end of next week, as I'm travelling.

Do you have some estimate on a release of 3.7.1? could this be included?

Rgds

@laoneo
Copy link
Member

laoneo commented May 5, 2017

Perhaps the maintainers can mark it as release blocker.

@rdeutz rdeutz self-assigned this May 5, 2017
@rdeutz rdeutz added this to the Joomla 3.7.1 milestone May 5, 2017
@Bakual
Copy link
Contributor

Bakual commented May 5, 2017

Just to be clear: This is not a major break as everything still works. It's just the layout that changes because the override isn't loaded anymore.
Technically changes in layouts aren't even covered by our B/C promise (see https://developer.joomla.org/development-strategy.html):

6.1.7 Rendered markup
For the time being, rendered markup is not subject to our backwards compatibility promise. We will try not to change markup in such a way that a site might render differently, but we can't promise not to break anything at the present time. We will work on defining ways in which we might make a backwards compatibility promise for markup in the future, but we do not currently have a satisfactory consensus on a workable standard.

@laoneo
Copy link
Member

laoneo commented May 5, 2017

This change is not about markup, I guess it is not even covered in the strategy.

@Bakual
Copy link
Contributor

Bakual commented May 5, 2017

Imho it's part of the rendered markup. But yeah, it's not explicitely stated. But then it doesn't matter as it is still not covered by the promise (there is only a promise for things listed there).

@rdeutz
Copy link
Contributor

rdeutz commented May 5, 2017

closing because we have a PR #15830

@rdeutz rdeutz closed this as completed May 5, 2017
@rdeutz rdeutz removed this from the Joomla 3.7.1 milestone May 5, 2017
@rdeutz rdeutz removed their assignment May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants