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 language options on non multilingual site #17086

Closed
wants to merge 15 commits into from

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Jul 12, 2017

Currently we have a column for the language on all list views and a field to select the language on all edit views

This PR removes them when you are on a non multilingual site. This simplifies the UI and removes a useless field when creating content

ToDo

After PR and non multilingual

screenshotr10-20-00

screenshotr10-20-55

Before PR and After PR and multilingual

screenshotr10-22-36

screenshotr10-22-46

@infograf768
Copy link
Member

infograf768 commented Jul 12, 2017

Once more: some 3pd may use other language systems to set a multilingual site.
In that case, they may not use the languagefilter plugin.
Therefore using if (JLanguageMultilang::isEnabled()) as a conditional would prevent users working with their extensions to see the language column or assihn content languages to items.

@infograf768
Copy link
Member

A possible solution to this is to create a new Global Configuration parameter.
Multilingual: Yes/no

Then we can

  1. OR force JLanguageMultilang::isEnabled() to include this parameter and create messages to inform users that enabling the languagefilter plugin is not enough to get the site multilingual, or manage to set that Global Parameter automatically when the plugin is enabled.

  2. OR create only this new parameter and then in your conditional, use
    `if (JLanguageMultilang::isEnabled() || JFactory::getConfig()->get('multilingual') == 1)

@brianteeman
Copy link
Contributor Author

This is j4. If they do it a non standard way then they will have to change. Joomla has an api and we must use it. We don't need any new parameters we have too many options as it is.

@infograf768
Copy link
Member

I guess 4.0 lead maintainers will take the decision. IMHO, Joomla should remain open.

@mbabker
Copy link
Contributor

mbabker commented Jul 12, 2017

We should have one API method to determine if multilingual functionality is available. Whether it's the core code or a third party extension, that code needs to be able to tell that API method the functionality is enabled. If that functionality is disabled, we should rightfully be able to remove things from the UI that would not be required without it.

So this is fine for 4.0. We aren't closing Joomla by any means. What this probably results in as things progress is changing our existing methods to make them more capable of working with third party solutions versus their current hardcoded manner of relying on one core plugin.

@brianteeman
Copy link
Contributor Author

Thank you for your wisdom @mbabker finally we can make progress on using joomla

@infograf768
Copy link
Member

What this probably results in as things progress is changing our existing methods to make them more capable of working with third party solutions versus their current hardcoded manner of relying on one core plugin.

Good. Then I guess that JLanguageMultilang::isEnabled() will have to change.

@mbabker
Copy link
Contributor

mbabker commented Jul 12, 2017

Yep. As a temporary thing (until we come up with a more eloquent solution), this would be the change I make:

diff --git a/libraries/src/CMS/Language/Multilanguage.php b/libraries/src/CMS/Language/Multilanguage.php
index d6d92b7..066280f 100644
--- a/libraries/src/CMS/Language/Multilanguage.php
+++ b/libraries/src/CMS/Language/Multilanguage.php
@@ -18,6 +18,14 @@ defined('JPATH_PLATFORM') or die;
 class Multilanguage
 {
 	/**
+	 * Flag indicating multilanguage functionality is enabled.
+	 *
+	 * @var    boolean
+	 * @since  __DEPLOY_VERSION__
+	 */
+	public static $enabled = false;
+
+	/**
 	 * Method to determine if the language filter plugin is enabled.
 	 * This works for both site and administrator.
 	 *
@@ -30,8 +38,11 @@ class Multilanguage
 		// Flag to avoid doing multiple database queries.
 		static $tested = false;
 
-		// Status of language filter plugin.
-		static $enabled = false;
+		// Do not proceed with testing if the flag is true
+		if (static::$enabled)
+		{
+			return true;
+		}
 
 		// Get application object.
 		$app = \JFactory::getApplication();
@@ -39,9 +50,9 @@ class Multilanguage
 		// If being called from the frontend, we can avoid the database query.
 		if ($app->isClient('site'))
 		{
-			$enabled = $app->getLanguageFilter();
+			static::$enabled = $app->getLanguageFilter();
 
-			return $enabled;
+			return static::$enabled;
 		}
 
 		// If already tested, don't test again.
@@ -57,11 +68,11 @@ class Multilanguage
 				->where($db->quoteName('element') . ' = ' . $db->quote('languagefilter'));
 			$db->setQuery($query);
 
-			$enabled = $db->loadResult();
+			static::$enabled = $db->loadResult();
 			$tested = true;
 		}
 
-		return (bool) $enabled;
+		return (bool) static::$enabled;
 	}
 
 	/**
@@ -110,3 +121,4 @@ class Multilanguage
 		return $multilangSiteHomePages;
 	}
 }
+

Then anyone can call JLanguageMultilang::$enabled = true; to turn on the system. If someone doesn't turn it on, then it falls back to the existing checks (which admittedly favors the core API and plugin).

@brianteeman
Copy link
Contributor Author

@wilsonge in order to complete the removal of the language sort and search filters we need something like #8767 so that we can conditionally remove them - can you help me out on that?

@brianteeman
Copy link
Contributor Author

Once more: some 3pd may use other language systems to set a multilingual site.

Can you please point to one of these systems as everything i tested on the jed uses the plugin

<th style="width:15%" class="nowrap">
<?php echo JText::_('JGRID_HEADING_LANGUAGE'); ?>
</th>
<?php if (JLanguageMultilang::isEnabled()) : ?>
Copy link
Member

Choose a reason for hiding this comment

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

No use. This component is not used if the plugin is disabled.

<th style="width:15%" class="nowrap">
<?php echo JText::_('JGRID_HEADING_LANGUAGE'); ?>
</th>
<?php if (JLanguageMultilang::isEnabled()) : ?>
Copy link
Member

Choose a reason for hiding this comment

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

No use. This component is not used if the plugin is disabled.

C-Lodder and others added 7 commits July 14, 2017 12:18
* Add support to view password in input field

* Add supporg for front-end login module

* Add support for admin login

* update tests
* Header icons styling

* Quickicons styling

* Background and color change to quickicons

* AA compliant for small text
* Move searchtools to dropdown

* Add eventlistener to body to close dropdown
* A11y admin login

* Update default.php

* Set JLOGIN string - Sentance case title
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Jul 14, 2017
@matrikular
Copy link
Contributor

Just a simple note: If we're going to do this, we would also have to make the colspan count for every list footer variable / dynamic.

Is there still time to split this into PRs on a "per extension basis" or at least to reduce the amount of files to review / test somehow? Personally, after the NNth file, I loose track.

What is the relation of the following code to this PR:
https://github.com/joomla/joomla-cms/pull/17086/files#diff-c59be36b2499577a7e6339384f5695c5R17

@brianteeman
Copy link
Contributor Author

Just a simple note: If we're going to do this, we would also have to make the colspan count for every list footer variable / dynamic.

The footer? Can you show me the code that is effected by this

What is the relation of the following code to this PR:

Thats an error in the PR from trying to solve conflicts and will be resolved if this is accepted

@matrikular
Copy link
Contributor

Sure.

This affects every list view like com_banners e.g.:
https://github.com/brianteeman/joomla-cms/blob/3723eed456ca12931411050f5a7ecc30f0a0f3ce/administrator/components/com_banners/tmpl/banners/default.php#L84

Looking at it, it seems that we have already an error in the colspan count. While there are 10 columns, the footer colspan is set to 13. With your PR, it should be 9 or 10 (depending on the language check).

The same goes for com_contact: https://github.com/brianteeman/joomla-cms/blob/3723eed456ca12931411050f5a7ecc30f0a0f3ce/administrator/components/com_contact/tmpl/contacts/default.php#L81

Or com_categories: https://github.com/brianteeman/joomla-cms/blob/3723eed456ca12931411050f5a7ecc30f0a0f3ce/administrator/components/com_categories/tmpl/categories/default.php#L136

Note that com_categories already uses a variable that is influenced by the icon columns.

@brianteeman
Copy link
Contributor Author

@matrikular thanks - I see what you mean now but as you said this error already exists before thiis PR and imho should be fixed in another PR

@matrikular
Copy link
Contributor

If you change the column count, you will have to change the colspan count as well. The fact that com_banners shows the wrong number has nothing to do with it.

@brianteeman
Copy link
Contributor Author

@matrikular from my research its not needed to reduce the colspan on the footer. As long as the colspan is equal to or greater than the number of columns it will span the entire width.

@brianteeman brianteeman added this to the Joomla 4.0 milestone Jul 19, 2017
@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Jul 31, 2017
@@ -94,8 +94,8 @@
<option value="impmade DESC">COM_BANNERS_HEADING_IMPRESSIONS_DESC</option>
<option value="clicks ASC">COM_BANNERS_HEADING_CLICKS_ASC</option>
<option value="clicks DESC">COM_BANNERS_HEADING_CLICKS_DESC</option>
<option value="a.language ASC">JGRID_HEADING_LANGUAGE_ASC</option>
<option value="a.language DESC">JGRID_HEADING_LANGUAGE_DESC</option>
<option value="a.language ASC" requires="adminlanguage">JGRID_HEADING_LANGUAGE_ASC</option></option>
Copy link
Contributor

Choose a reason for hiding this comment

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

can you double check this lines? IMO there is a closing </option> too much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i need to review it all when I get home as i made a silly mistake anyway

@brianteeman
Copy link
Contributor Author

closed temperarily

@brianteeman brianteeman reopened this Aug 24, 2017
@brianteeman
Copy link
Contributor Author

re-opened to resolve conflicts

@brianteeman
Copy link
Contributor Author

closed as the core code has changed too much for an easy merge/update - will redo it at some point

@brianteeman
Copy link
Contributor Author

See #17765

@brianteeman brianteeman deleted the ismulti branch August 30, 2017 00:00
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.

8 participants