Skip to content

Conversation

@brianteeman
Copy link
Contributor

Pull Request for Issue ##33173.

Summary of Changes

Adds an additional message which contains the link to the templates component

Testing Instructions

I honestly have no idea

@particthistle
Copy link
Member

particthistle commented Apr 23, 2021

Yes, not an easy core one to test. Need to create a test component to install with an override to trigger something... or find a need to modify a template item in Cassiopeia.

@Anu1601CS do you have any test components that you had floating around from when you created the override check and comparison functionality?

@PhilETaylor

This comment was marked as abuse.

@brianteeman brianteeman changed the title first commit [4.0] On updating Joomla, "Overridden files have changed" message should be linked to allow resolution. Apr 23, 2021
@richard67
Copy link
Member

Regarding testing: How about using patchtester? Install version 4.0.0, make some override for it, and then update to 4.0.1?

@brianteeman
Copy link
Contributor Author

@richard67 Might work. I was trying to test it with core last night but you cant because the file was replaced in the core update. Will test again myself later today

@richard67
Copy link
Member

Regarding testing: How about using patchtester? Install version 4.0.0, make some override for it, and then update to 4.0.1?

Hmm, no, doesn't work. No overrides can be made for the patchtester. Have to find another one.

@richard67
Copy link
Member

2021-04-23_1

My review suggestion above fixes that.

@richard67
Copy link
Member

@brianteeman I've just tested with a 3rd party admin module. Both the link added with this PR as well as the Quickicon for the overrides check point to site templates, but they should point to admin templates in case of an admin module.

Do you think this can be fixed for your link with this PR here, and then for the Quickicon with another PR?

Or should I ignore that and give this PR a good test result?

@richard67
Copy link
Member

For testing I have used https://www.phoca.cz/download/category/123-phoca-top-menu-module . There is a glitch with the Override Check not related to this PR: I did not get the alert modified by this PR when updating that module from 4.0.4. to 4.0.5. So I have uninstalled that extension after I had created the override, and then installed it again, and then I could see that alert.

@brianteeman
Copy link
Contributor Author

I would say thats unrelated. but a genuine issue with the override check

@richard67
Copy link
Member

I would say thats unrelated. but a genuine issue with the override check

@brianteeman Does that refer to my last comment about the glitch? Then I agree with you. Or does it refer to my comment before that one about link always going to site templates, also for admin modules? In this case I have to think about it.

@richard67
Copy link
Member

In the #__template_overrides table in database is information about the template, so it should be possible to check if it's a site or an admin template. But maybe that should be done with another, future PR, at all places, including the one being added here.

@brianteeman
Copy link
Contributor Author

The one about the site templates. If the quickicon also goes there (and I suspect it does) then its a bigger bug than addressed here

@brianteeman
Copy link
Contributor Author

If this PR goes to the same url as the quickicon then this PR is correct and anything else should be its own issue

@richard67
Copy link
Member

I have tested this item ✅ successfully on 320fa88

Tested with use of https://www.phoca.cz/download/category/123-phoca-top-menu-module .

There is a glitch with the Override Check not related to this PR: I did not get the alert modified by this PR when updating that module from 4.0.4. to 4.0.5. So I have uninstalled that extension after I had created the override, and then installed it again, and then I could see that alert.

The link in the alert added by this PR has the same target as the Quickicon for the Override Checks has. Both have the problem pointing to site templates in any case, also if the override was made in an admin template e.g. for an admin module.

So this PR is consistent with the Quickicon.

The other issues mentioned above need to be fixed separately.


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

@richard67
Copy link
Member

I have not tested this item.

Sorry, my previous test was wrong.

The quickicon correctly points to admin templates if the override was made in an admin template, and so should do the link added by this PR.


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

@richard67
Copy link
Member

Hmm, now after having logged out and in again, the Quickicon points to site templates, which is wrong again.

@richard67
Copy link
Member

The more I think about it, the less I know what's the right way. What if one has overrides in site and in admin templates, where should that link go to?

I think this PR is ok so far as it solve the issue it refers to and is an improvement, and other glitches have to be solved separately.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 320fa88


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

@richard67
Copy link
Member

It seems that both the link added here and the quickicon go to either admin or site templates depending on which of these you have visited the last time before in your session, i.e. if you once have been in the admin templates view and now use the link or the quickicon, you get to the admin templates, but after logout and login again it goes to site templates. I have some vague memory that this was subject of some issue or some comment in some issue or PR, but I am not sure. Maybe someone else remembers better if that is a known issue?

@brianteeman
Copy link
Contributor Author

It was for editing modules

@Quy
Copy link
Contributor

Quy commented Jul 7, 2021

33256

@brianteeman
Copy link
Contributor Author

What is the output when language debug is NOT enabled?

@Quy
Copy link
Contributor

Quy commented Jul 7, 2021

33256-no-debug

@brianteeman
Copy link
Contributor Author

Closing as the more I look at it i dont think this is the correct approach

@brianteeman brianteeman deleted the over branch July 13, 2021 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants