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

[3.10] Add a check whether we are running the core backend template and issue an info when that is not the case #35107

Merged
merged 36 commits into from Nov 26, 2021

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Aug 12, 2021

Summary of Changes

Add a check whether we are running the core tempaltes where the backend it required and the frontend is recommended

Testing Instructions

Actual result BEFORE applying this Pull Request

No checks for the default templates

image

Expected result AFTER applying this Pull Request

image

Documentation Changes Required

https://docs.joomla.org/Pre-Update_Check - Has to be updated

TTs have to be informed about the language stirngs

cc @joomla/cms-release-team @HLeithner @bembelimen

…nd it required and the frontend is recommended
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-3.10-dev labels Aug 12, 2021
@brianteeman
Copy link
Contributor

These are NOT php or database settings

Core backend template is NOT a requirement

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 33a0aed


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

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 12, 2021

Core backend template is NOT a requirement

Will move it to the optional thing than fine for me.

These are NOT php or database settings

Agree its on my list to be updated too.

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 12, 2021

The requested changes have been pushed. They are now both under "the seccond box" and the title has been updated too.

@brianteeman
Copy link
Contributor

why check for isis? there is more than 1 admin template

@brianteeman
Copy link
Contributor

Neither of these checks are anything to do with compatibility. They are only really needed to perhaps make the update easier. This is just creating FUD

@richard67
Copy link
Member

Here in the 4.0 update SQL we check for hathor and isis and update to atum if one of these is used: https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_admin/sql/updates/mysql/4.0.0-2018-03-05.sql#L23

And one row below we check for protostar and beez 3 for the frontend.

So why do the language strings added by this PR here not mention hathor or beez3? It should work same well as with the other core templates.

We should not provide false information in language strings which are made to guide people.

@brianteeman
Copy link
Contributor

This and the other PR should be closed and re-evaluated. There is ZERO reason for either PR.

Not withstanding the code issues the concept of both PR is wrong!! They only succeed in spreading fear, uncertainty and doubt about the upgrade process.

I can't believe you are even remotely thinking about such a change after the final release candidate.

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 164496e


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

zero-24 and others added 2 commits August 13, 2021 00:17
Co-authored-by: Brian Teeman <brian@teeman.net>
@zero-24
Copy link
Contributor Author

zero-24 commented Aug 12, 2021

So why do the language strings added by this PR here not mention hathor or beez3? It should work same well as with the other core templates.

Isnt hathor gone already? I can add them for sure.

I can't believe you are even remotely thinking about such a change after the final release candidate.

It has been raised by the @joomla/cms-release-team that there should be messages like that. I see where an "pre upgrade checker" could warn about such issues that affect the site performance after the upgrade or the upgrade process.

For example an non-core backed templates or non-core frotend template could break the site once upgraded so the recommendation would be for the time of the upgrade to switch to the core where we know that the upgrade works and they are beeing replaced by 4.x compatible code than you can go and enable the other stuff step by step too.

The router stuff is something different happening under the hood so the idea was to tell the people that there could be changes in the URLs joomla generates after the upgrade.

@brianteeman
Copy link
Contributor

Isnt hathor gone already?

No - just not supported. You obviously didnt even check. https://github.com/joomla/joomla-cms/tree/3.10-dev/administrator/templates

It has been raised by the @joomla/cms-release-team that there should be messages like that.

Excuse my language but "a bit bloody late"

For example an non-core backed templates or non-core frotend template could break the site once upgraded so the recommendation would be for the time of the upgrade to switch to the core where we know that the upgrade works and they are beeing replaced by 4.x compatible code than you can go and enable the other stuff step by step too.

But that is not what you are doing or saying with these pr

The router stuff is something different happening under the hood so the idea was to tell the people that there could be changes in the URLs joomla generates after the upgrade.

Again that is not what is being done or said with these PR.

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 12, 2021

No - just not supported. You obviously didnt even check. https://github.com/joomla/joomla-cms/tree/3.10-dev/administrator/templates

Hmm I'm sure we did a postinstall for hathor users but yes seems its still shipped.

But that is not what you are doing or saying with these pr

Than its great that you raised this questions so that this can be clarified. What can be done to do what I'm trying to do?

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 9, 2021

Will leave it for today, but want to go back later to Isis and J 3.10.1 again :-)

The message seems to be correct to me, so this would be a successfull test. :)

@ChristineWk
Copy link

I have tested this item ✅ successfully on ad7b419

But it's not in blue :-)
Would also prefer to switch to Isis before updating. Similar to the frontend template on Protostar (if you were on a copy)


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

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 11, 2021

But it's not in blue :-)

I have just double checked that, it seems the template does not show "info" as blue. But isis does so it seems the template decided to show information messages in yellow which is a chooise of the template.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 11, 2021

Sorry but the warning message position is completely the wrong place for it in my opinion.

I'm sorry @HLeithner seems I have missed this comment. Do you have another proposal where that message should be shown while it should only be shown too a small number of sites anyway right?

@zero-24 zero-24 modified the milestones: Joomla 3.10.2, Joomla 3.10.3 Sep 11, 2021
@HLeithner
Copy link
Member

like all other warnings, maybe in a new section.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 11, 2021

like all other warnings, maybe in a new section.

What kind of new section do you have in mind?

@zero-24 zero-24 modified the milestones: Joomla 3.10.3, Joomla 3.10.4 Oct 15, 2021
@zero-24
Copy link
Contributor Author

zero-24 commented Nov 21, 2021

@HLeithner can we get a response here so the PR can be adjusted or closed based on the feedback?

@HLeithner
Copy link
Member

I thought about something like "Required PHP & Database Settings".

@zero-24
Copy link
Contributor Author

zero-24 commented Nov 21, 2021

Is it required or an php or database setting? This is exactly the place where all of this started but it was moved outside of this checks for that reason and moved into a standalone blue message.

@HLeithner
Copy link
Member

not in this box, a new box like this box

@zero-24
Copy link
Contributor Author

zero-24 commented Nov 21, 2021

And how would you call it, what would we show when you are running isis? Just as a reminder its only showed when you have a non-isis backend template?

@zero-24
Copy link
Contributor Author

zero-24 commented Nov 26, 2021

Will take this now in as it is for now. When we find better ways to display it I'm open to change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants