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

Change language debug mode to allow it to show the language constants #21691

Merged
merged 8 commits into from Sep 8, 2018

Conversation

brianteeman
Copy link
Contributor

PR for #14304

By default there is no change when debug language is enabled - but there now is an option to display either the language constant or the language value

Arguably a new feature - I will let the maintainer decide on that

PR for joomla#14304

By default there is no change when debug language is enabled - but there now is an option to display either the language constant or the language value
PR for joomla#13904

### Steps to reproduce the issue
Extensions > Modules > Go to any module's settings
Module ordering field looks different.

### Expected result
All select fields must looks the same.

### Actual result
![module-settings-ordering](https://cloud.githubusercontent.com/assets/1245155/22618818/9c8a2dd6-eaf7-11e6-8cc8-06a9ae610849.png)

### After pr
all select fields look the same
@elkuku
Copy link
Contributor

elkuku commented Aug 18, 2018

I have tested this item ✅ successfully on 1277199

There is an issue that debug settings are not immediately being applied so I guess testers might get confused?

Anyway - works 😉


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

$string = $this->debug ? '**' . $this->strings[$key] . '**' : $this->strings[$key];
$value = $this->debug && \JFactory::getApplication()->get('debug_lang_const') == 0 ? $key : $this->strings[$key];

$string = $this->debug ? '**' . $value . '**' : $this->strings[$key];
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 propose to refactor this a bit, so the code is only executed when debugging - this should have a measurable impact!

if (isset($this->strings[$key]))
{
	$string = $this->strings[$key];				

	// Store debug information
	if ($this->debug)
	{
		$value = \JFactory::getApplication()->get('debug_lang_const') == 0 ? $key : $string;
		$string = '**' . $value . '**';
				
		$caller = $this->getCallerInfo();
		if (!array_key_exists($key, $this->used))
		{
			$this->used[$key] = array();
		}
		$this->used[$key][] = $caller;
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied your changes

@elkuku
Copy link
Contributor

elkuku commented Aug 19, 2018

May I politely ask a question here.. in case this get merged... will the code eventually also end up in the 4.0 branch or does this require a separate PR?

@brianteeman
Copy link
Contributor Author

basically yes it should do as george is pushing them all up (eventually)

however as this one uses external libraries iirc it might need to be changed so he might ask you to do a specific pr as we handle those differently in j4

@elkuku
Copy link
Contributor

elkuku commented Aug 19, 2018

he might ask you to do a specific pr

I'd love to do that.

Thanks!

@brianteeman
Copy link
Contributor Author

@elkuku I have no idea what I was talking about above. You dont have to do anything. If accepted then @wilsonge will eventually merge it into j4 as well. I "think" I replied on my phone and thought you were commenting on your own pr for a new debugger

@elkuku
Copy link
Contributor

elkuku commented Aug 24, 2018

You dont have to do anything.

I also love to do that 😉

Thanks for the update.

@infograf768
Copy link
Member

Works fine, but the Save toolbar button may be an issue when switching from constant to value.

screen shot 2018-08-25 at 07 45 39

screen shot 2018-08-25 at 07 46 15

and even worse when there is a specific custom admin menu added.

@brianteeman
Copy link
Contributor Author

That is unavoidable if someone wants this feature.

@elkuku
Copy link
Contributor

elkuku commented Aug 31, 2018

I have tested this item ✅ successfully on 82fd8ed


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

@infograf768
Copy link
Member

I suggest to add an onChange event in the form field to save and reload the page.

@brianteeman
Copy link
Contributor Author

That would then be different to every other selector in the ui - so that would not be a good idea

@tecpromotion
Copy link
Contributor

I have tested this item ✅ successfully on 82fd8ed

Tested successfully.


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

@ghost
Copy link

ghost commented Sep 8, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 8, 2018
@mbabker
Copy link
Contributor

mbabker commented Sep 8, 2018

I'm not going to block the merge on this but we need to update the Language class to not rely on pulling this param from the global config at all times (basically, the same way the class has a parameter to enable/disable debug mode we should have the same for this new capability).

@mbabker mbabker merged commit 09c23ca into joomla:staging Sep 8, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 8, 2018
@mbabker mbabker added this to the Joomla 3.9.0 milestone Sep 8, 2018
@brianteeman
Copy link
Contributor Author

Thanks for merging. Not sure I can do that change but I will try

@infograf768
Copy link
Member

This created an issue when debug is on when installing Joomla
See #28913

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.

None yet

7 participants