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

[4.0] Remove mod_multilangstatus tampering from mod_user #30113

Merged
merged 8 commits into from
Nov 7, 2020
Merged

[4.0] Remove mod_multilangstatus tampering from mod_user #30113

merged 8 commits into from
Nov 7, 2020

Conversation

SharkyKZ
Copy link
Contributor

@SharkyKZ SharkyKZ commented Jul 16, 2020

Closes #29948.

Summary of Changes

Removes mod_multilangstatus tampering from mod_user.

Testing Instructions

Create and publish a Multilingual Status module.

Actual result BEFORE applying this Pull Request

Module created but keeps getting unpublished on every page load.

Expected result AFTER applying this Pull Request

Module created and published.

Documentation Changes Required

Joomla\Module\Multilangstatus\Administrator\Helper\MultilangstatusAdminHelper class is removed. If it's too late for that, it can be deprecated instead.

@infograf768
Copy link
Member

As far as I understand, this breaks the automatic display of the module when multilang is ON.
I believe this was a real improvement in J4 for multilingual sites.

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Jul 16, 2020

Yes, it stops unpublished modules being automatically published.

@infograf768
Copy link
Member

I can't be in favor of that. As I said, this is definitely an improvement for all multilingual sites.

@SharkyKZ
Copy link
Contributor Author

Hardcode it into the template if you want. Just stop messing with user data.

@infograf768
Copy link
Member

We have enough explaining all the time in the forums how to use that module first when someone has a multilingual issue.

I was not the one who originally created that code and I don't know how to hard code it in atum.
This basically means that, if you want to get rid of it from mod_user, I am fine with it, but please provide also the solution to hardcode it in atum.

@brianteeman
Copy link
Contributor

I agree with @infograf768 Dont just remove functionality without providing a replacement

@SharkyKZ
Copy link
Contributor Author

Personally, I don't think there is a need for replacement for this. If you know how to enable Language Filter plugin, publishing the module shouldn't be that hard. But OK, some options:

  1. Have the module published on all new sites.
  2. Have the module published when installing multilingual sample data.
  3. Have the module published when enabling Language Filter plugin.
  4. Have the module embedded in the template.

Take your pick. Anything is better than what we have now.

bug2

@Bakual
Copy link
Contributor

Bakual commented Jul 16, 2020

Why don't we have the module enabled by default, but display nothing when multilang is disabled? We have such behavior regulary in modules. Then it would automatically appear as soon as multilang is enabled. That would be the best approach imho.

A module should never automatically change its state. If it's unpublished, it has to stay unpublished, if it's published, it has to stay published. Also modules shouldn't interact with other modules, that's just plain wrong.
It should also not be hardcoded into the template, that would be equally wrong.

@Bakual
Copy link
Contributor

Bakual commented Jul 16, 2020

Just for reference a PR which I did last year. Turns out the code was just moved in between and the PR mistakenly closed.
#24341
It's more or less exactly the same as this one here.

@Bakual
Copy link
Contributor

Bakual commented Jul 16, 2020

@SharkyKZ If you add the following lines to administrator/modules/mod_multilangstatus/mod_multilangstatus.php at line 13 (after the use statement), then you got option 1 but the module doesn't show up until the language filter plugin is enabled. Similar to other modules that don't show up if there is nothing to show.

use Joomla\CMS\Language\Multilanguage;

if (!Multilanguage::isEnabled())
{
	return;
}

@SharkyKZ
Copy link
Contributor Author

PR updated. Using the check from com_languages.

@infograf768
Copy link
Member

I don't understand anything to your changes. They look much more complex than our suggestion. Any reason to choose such a code?
if you insist on this, please correct typo $mutilanguageEnabled to $multilanguageEnabled

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Jul 17, 2020

It's the same but with dependencies passed to Multilanguage::isEnabled(). Typo fixed.

@infograf768
Copy link
Member

Do you mean that we should change everywhere Multilanguage::isEnabled() to Multilanguage::isEnabled($app, Factory::getContainer()->get(DatabaseInterface::class)); ?

A simple grep gives me 134 occurrences of that code present in 88 files in J4

@SharkyKZ
Copy link
Contributor Author

That seems to be the way going forward. But there's no need to rush replacing every existing instance now.

@richard67
Copy link
Member

@wilsonge Any opinion on the class removal (or deprecation) mentioned in the "Documentation Changes Required" section of the description?

@infograf768
Copy link
Member

IMHO, as it does not exist in J3, no need to deprecate it.
People who are using beta versions should only use them for tests.

@vijaykhollam
Copy link
Contributor

I have tested this item ✅ successfully on f2ff1d4


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

1 similar comment
@Kaustubharas
Copy link

I have tested this item ✅ successfully on f2ff1d4


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

@alikon
Copy link
Contributor

alikon commented Nov 7, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 7, 2020
@HLeithner HLeithner merged commit 994c9d0 into joomla:4.0-dev Nov 7, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 7, 2020
@HLeithner
Copy link
Member

Thanks

@SharkyKZ SharkyKZ deleted the j4/cleanup/mod-user branch November 7, 2020 12:34
@zero-24 zero-24 added this to the Joomla 4.0 milestone Nov 7, 2020
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.