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

Make metadata.xml in view optional. #7654

Merged
merged 4 commits into from Oct 17, 2015
Merged

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented Aug 7, 2015

Issue

As explained in #7649, currently we are required to have a metadata.xml present in the view folder and that file needs to have a title specified.
But that title isn't used for anything, the menumanager (items view) only uses the title from the layout.
If the metadata.xml isn't present, the menumanager will not show the layout title but instead shows the name (as it appears in the URL!) of the view.
See picture for what I mean
menumanager

Solution

This PR proposes to make the whole file optional and as well the title attribute. If the title is specified and it is a translated string, it assumes that the developer wants to show the view title in the menu manager. The following table tries to show what changed. View and Layout column refers to if the file is present and contains a title.

View Layout Before After
X X Component » Layout Component » View » Layout
X - Component Component » View
- X Component » Untranslated Viewname (URL) Component » Layout
- - Component » Untranslated Viewname (URL) Component » Untranslated Viewname (URL)

Backward Compatibility

As you see in the above table, there is some difference in behavior. Hopefully you all agree that it makes more sense now 😄
To avoid showing now untranslated view strings (remember, those are currently required to be in the view metadata,xml!), I added a check into that part which only shows the view title if the string is in the language file.
You can test this easily with menu items from com_content. All metadata.xml files from this extension contain an untranslated title there. So those should not show up after the PR is applied.

Testing

After applying patch, there should be no difference for most extensions.
The only core extension which will have a difference is com_wrapper. Instead of "Wrapper » Iframe Wrapper" it will now show "Wrapper » Wrapper » Iframe Wrapper".
Com_content should behave the same because the view title isn't translated.
To test, you can delete the metadata.xml of a given view and see if something changes. For the wrapper, the view should vanish and only the layout show. For com_content menuitems there should be no change.
If you delete both the xml for the layout (usually default.xml) and the view (metadata.xml) you will see that untranslated viewname from the URL.

@brianteeman
Copy link
Contributor

brianteeman commented Aug 7, 2015 via email

$item->item_type_desc = JText::_(trim((string) $layout[0]->message[0]));
}
JLog::add(
'Using an untranslated string ("' . $viewTitle . '") as title in the view manifest (' . $file . ') is deprecated. Please either translate or remove it.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be weary about enforcing this. It makes a requirement that extensions hooking into com_menus MUST use translations and the JLanguage API. Today it is possible to deploy extensions without using language files (although there are places in the UI where the display is less than optimal). How does the behavior with the forced language string requirement improve the application?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that I need to check if the string is translated here, because otherwise the view title would show up for all extensions (because it currently is required to be there), which is certainly not what we want.
I assumed if someone translated the title, then it's meant to be shown and doesn't create any issues.

I can of course remove the deprecation notice. And with J4 we can remove the check if the title is translated. That would the other way which would be B/C.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way the deprecation notice is worded it makes it seem like it will be a requirement in J4 that the title must use a translation string, not something we want to force IMO. As long as things can still work without the language key I would just drop the notice and leave an inline note in the code saying what part will be changed in J4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I removed the notice now and adjusted the comment.

@Bakual
Copy link
Contributor Author

Bakual commented Aug 8, 2015

Excuse me for being thick but what is the benefit of showing wrapper-wrapper-iframewrapper

There is no benefit here. It could easily be removed by removing the title from the metadata.xml.
It was there before and because it is actually translated (but never was shown before) it now shows up.
I left it there so it's easier to test the PR. I can remove it in another PR.

@piotr-cz
Copy link
Contributor

I was always super confused about this file.
And even more about the metadata.xml file straight in some component directories.

@Bakual
Copy link
Contributor Author

Bakual commented Aug 13, 2015

I was always super confused about this file.
And even more about the metadata.xml file straight in some component directories.

It's actually a nice feature. That metadata.xml can contain parameters for a view instead of adding it to the layout files. In my own component I use that to avoid having to copy-paste each parameter for each layout (my layout files are just showing the same data in different layouts).

As for the metadata.xml in the component directory, that one is useless in that case. However it could be used to specify parameters which then would be used component wide (if no view or layout xml is found).

@piotr-cz
Copy link
Contributor

I'd vote for removal of components metadata.xml file if it's not doing anything ATM. At least for me it's confusing and I spent some time to reverse-engineer Joomla code to find out what is it for (nothing).

Ony when there is some functionality implemented, I'd reintroduce it again.

@Bakual
Copy link
Contributor Author

Bakual commented Aug 14, 2015

Feel free to propose a PR which removes the (empty) component metadata.xml from core where present. It's not in the scope of this PR. :)
It's just useless if there are no params defined in it, like the one you linked. If you have params, the code currently supports it.

@Harmageddon
Copy link
Contributor

I have tested this item ✅ successfully on

Edit: The Issues tool cut my comment...
Tested with the "All Frontend Views" menu from the testing sample data as well as with an own component that has no metadata.xml files. Looks fine to me.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7654.

@zero-24
Copy link
Contributor

zero-24 commented Oct 15, 2015

@Bakual can you sync to fix the conflicts here?

Conflicts:
	administrator/components/com_menus/views/items/view.html.php
@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @Harmageddon


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7654.

@Bakual
Copy link
Contributor Author

Bakual commented Oct 15, 2015

@zero-24 As you wish 😄

@zero-24
Copy link
Contributor

zero-24 commented Oct 15, 2015

I have tested this item ✅ successfully on 806c549

I can confirm that the behavior for com_wrapper is changed. ;)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7654.

@Harmageddon
Copy link
Contributor

I have tested this item ✅ successfully on 806c549

Tested again as before.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7654.

@zero-24 zero-24 added this to the Joomla! 3.4.5 milestone Oct 15, 2015
@zero-24
Copy link
Contributor

zero-24 commented Oct 15, 2015

RTC. thanks


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7654.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 15, 2015
rdeutz added a commit that referenced this pull request Oct 17, 2015
Make metadata.xml in view optional.
@rdeutz rdeutz merged commit b630787 into joomla:staging Oct 17, 2015
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 17, 2015
@Bakual Bakual deleted the MenuManager2 branch October 17, 2015 15:57
@zero-24 zero-24 modified the milestones: Joomla! 3.4.6, Joomla! 3.5.0 Oct 28, 2015
@zero-24 zero-24 removed this from the Joomla! 3.4.6 milestone Oct 28, 2015
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

Successfully merging this pull request may close these issues.

None yet

8 participants