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

commented Aug 18, 2018

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 #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
brianteeman added 2 commits Aug 18, 2018
PR for #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
This reverts commit 7a1cf2d.
@elkuku

This comment has been minimized.

Copy link
Member

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];

This comment has been minimized.

Copy link
@elkuku

elkuku Aug 18, 2018

Member

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;
	}
}

This comment has been minimized.

Copy link
@brianteeman

brianteeman Aug 24, 2018

Author Contributor

applied your changes

@elkuku

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

he might ask you to do a specific pr

I'd love to do that.

Thanks!

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2018

@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

brianteeman and others added 5 commits Aug 24, 2018
@elkuku

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

You dont have to do anything.

I also love to do that 😉

Thanks for the update.

@infograf768

This comment has been minimized.

Copy link
Member

commented Aug 25, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2018

That is unavoidable if someone wants this feature.

@elkuku

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

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

@brianteeman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

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

@tecpromotion

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2018

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.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Sep 8, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Sep 8, 2018
@mbabker

This comment has been minimized.

Copy link
Member

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
5 checks passed
5 checks passed
Hound No violations found. Woof!
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joomla-cms-bot joomla-cms-bot removed the RTC label Sep 8, 2018
@mbabker mbabker added this to the Joomla 3.9.0 milestone Sep 8, 2018
@brianteeman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2018

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

@brianteeman brianteeman deleted the brianteeman:debug-lang branch Sep 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.