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.0 remove error translation #2588

Merged
merged 2 commits into from Jan 4, 2014
Merged

3.0 remove error translation #2588

merged 2 commits into from Jan 4, 2014

Conversation

lorenzo
Copy link
Member

@lorenzo lorenzo commented Jan 2, 2014

This takes care of ticket #2586

Feel free to merge once it is approved there

@ADmad
Copy link
Member

ADmad commented Jan 2, 2014

Builds failing.

trigger_error(__d(
'cake_dev', 'Could not apply permission mask "%s" on cache file "%s"',
trigger_error(sprintf(
'Could not apply permission mask "%s" on cache file "%s"',
[$this->_File->getPathname(), $this->_config['mask']]), E_USER_WARNING);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this array wrapping should stay.

@lorenzo
Copy link
Member Author

lorenzo commented Jan 2, 2014

If anyone is willing to add extra commits to this branch please go ahead. I'll be out for some time and will not be able to take care of the new comments until tomorrow

<strong><?php echo __d('cake_dev', 'Error'); ?>: </strong>
<?php echo __d('cake_dev', '%s could not be found.', '<em>' . h($pluginDot . $class) . '</em>'); ?>
<strong>Error: </strong>
<?php echo sprintf('%s could not be found.', '<em>' . h($pluginDot . $class) . '</em>'); ?>
Copy link
Member

Choose a reason for hiding this comment

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

the markup here should be in the replacement string or.. no sprintf usage and only

text <?= h($pluginDot . $class) ?> 

@AD7six
Copy link
Member

AD7six commented Jan 2, 2014

Man that looks like it was tedious to do 👏

There are a few recurring things that I think should be fixed before merging, but if they aren't 👍 anyway:

  • sprintf usage with literal strings as replacements
  • replacement markers which contain html markup - now the markup is more appropriate in the message
  • in some cases changing from sprintf to static html and dumping the variable with <?= $foo ?> inline may lead to cleaner files.

I'll try and address some of them.

@AD7six
Copy link
Member

AD7six commented Jan 3, 2014

I've squashed everything together ontop of 3.0 and addressed the conflicts - because it touches so many files would be awesome to review and merge as soon as travis says A-OK.

trigger_error(__d(
'cake_dev', 'Could not apply permission mask "%s" on log file "%s"',
trigger_error(sprintf(
'Could not apply permission mask "%s" on log file "%s"',
array($this->_config['mask'], $pathname)), E_USER_WARNING);
Copy link
Member

Choose a reason for hiding this comment

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

The array needs to be removed here.

ADmad added a commit that referenced this pull request Jan 4, 2014
@ADmad ADmad merged commit 1f8696a into 3.0 Jan 4, 2014
@ADmad ADmad deleted the 3.0-remove-error-translation branch January 4, 2014 07:40
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.

None yet

4 participants