First batch of module tmpl codestyle #17851

zero-24 · 2017-09-03T20:38:48Z

Summary of Changes

First batch of module tmpl codestyle

Testing Instructions

Please test and review the following frontend modules using the testing sampledata

mod_articles_archive
mod_articles_categories
mod_articles_category
mod_articles_latest
mod_articles_news
mod_articles_popular
mod_banners
mod_breadcrumbs
mod_custom

Expected result

Still works

Actual result

Works with codestyle problems

Documentation Changes Required

none

Quy · 2017-09-03T20:50:32Z

modules/mod_banners/tmpl/default.php

+								title="<?php echo htmlspecialchars($item->name, ENT_QUOTES, 'UTF-8'); ?>">
+								<img
+									src="<?php echo $baseurl . $imageurl; ?>"
+									alt="<?php echo $alt;?>"


Add space before ?>". Do the same for the following 2 lines.

Quy · 2017-09-03T20:50:55Z

modules/mod_banners/tmpl/default.php

+								title="<?php echo htmlspecialchars($item->name, ENT_QUOTES, 'UTF-8'); ?>">
+								<img
+									src="<?php echo $baseurl . $imageurl; ?>"
+									alt="<?php echo $alt;?>"


Add space before ?>". Do the same for the following 2 lines.

Quy · 2017-09-03T20:51:15Z

modules/mod_banners/tmpl/default.php

+								<img
+									src="<?php echo $baseurl . $imageurl; ?>"
+									alt="<?php echo $alt;?>"
+									<?php if (!empty($width)) echo ' width="' . $width . '"';?>


Add space before ?>". Do the same for the following 2 lines.

Quy · 2017-09-03T20:51:52Z

modules/mod_articles_news/tmpl/_item.php

 <?php if (isset($item->link) && $item->readmore != 0 && $params->get('readmore')) : ?>
-	<?php echo '<a class="readmore" href="' . $item->link . '">' . $item->linkText . '</a>'; ?>
+	<a class="readmore" href="'<?php echo $item->link; ?>'">


Remove ' before and after the php code

Quy · 2017-09-03T20:53:26Z

modules/mod_banners/tmpl/default.php

+							loop="false"
+							pluginspage="http://www.macromedia.com/go/get/flashplayer"
+							type="application/x-shockwave-flash"
+							<?php if (!empty($width)) echo ' width="' . $width . '"';?>


Add space before ?>. Do the same below.

Quy · 2017-09-03T20:53:48Z

modules/mod_banners/tmpl/default.php

+						<?php // Just display the image if no link specified ?>
+						<img
+							src="<?php echo $baseurl . $imageurl; ?>"
+							alt="<?php echo $alt;?>"


Add space before ?>". Do the same for the following 2 lines.

dgrammatiko · 2017-09-03T21:04:39Z

@zero-24 you're killing us, do it in J4...

brianteeman · 2017-09-03T21:10:00Z

modules/mod_breadcrumbs/tmpl/default.php

 				<?php if (($key !== $penult_item_key) || $show_last) : ?>
 					<span class="divider">
 						<?php echo $separator; ?>
 					</span>
 				<?php endif; ?>
-				<meta itemprop="position" content="<?php echo $key + 1; ?>">
+				<meta itemprop="position" content="<?php echo $key + 1; ?>" />


you have added a closing / here but this is not required for html5

mbabker · 2017-09-03T21:11:58Z

you're killing us, do it in J4...

It'd actually be better if the code style were consistent across branches. If you're making massive code style changes in only one branch you're making even more merge conflict issues as time goes on.

zero-24 · 2017-09-03T21:13:29Z

It'd actually be better if the code style were consistent across branches. If you're making massive code style changes in only one branch you're making even more merge conflict issues as time goes on.

So what is your suggestion? Doing it against staging or 4.0?

mbabker · 2017-09-03T21:14:26Z

Keep going against staging. In theory there should only be one merge that has major conflicts, every merge after that should only deal with lines changed between the two branches.

Quy · 2017-09-04T02:06:43Z

modules/mod_articles_categories/tmpl/default_items.php

-
+			<a href="<?php echo JRoute::_(ContentHelperRoute::getCategoryRoute($item->id)); ?>">
+				<?php echo $item->title; ?>
+					<?php if ($params->get('numitems')) : ?>


Lines 18-20: Remove tab

Quy · 2017-09-04T02:10:02Z

modules/mod_articles_category/tmpl/default.php

-										<?php echo $readmore; ?>
-										<?php echo JHtml::_('string.truncate', $item->title, $params->get('readmore_limit')); ?>
-											<?php if ($params->get('show_readmore_title', 0) != 0) : ?>
-												<?php echo JHtml::_('string.truncate', $this->item->title, $params->get('readmore_limit')); ?>


Lines 66-68: Don't indent?

Quy · 2017-09-04T02:10:39Z

modules/mod_articles_category/tmpl/default.php

+							<?php endif; ?>
+							<?php if ($params->get('show_readmore')) : ?>
+								<p class="mod-articles-category-readmore">
+									<a class="mod-articles-category-title <?php echo $item->active; ?>" href="<?php echo $item->link; ?>">


Multiple lines

hmm can you please doube check that? Looks good to me?

Quy · 2017-09-05T03:22:02Z

modules/mod_articles_category/tmpl/default.php

+								</span>
+							<?php endif; ?>
+							<?php if ($item->displayDate) : ?>
+								<span class="mod-articles-category-date"><?php echo $item->displayDate; ?></span>


Multiple lines.

Quy · 2017-09-05T03:29:02Z

modules/mod_custom/tmpl/default.php


+?>
 <div class="custom<?php echo $moduleclass_sfx; ?>" <?php if ($params->get('backgroundimage')) : ?> style="background-image:url(<?php echo $params->get('backgroundimage'); ?>)"<?php endif; ?> >


Remove space > >

Quy · 2017-09-06T05:21:41Z

modules/mod_articles_categories/tmpl/default_items.php

@@ -9,29 +9,29 @@

 defined('_JEXEC') or die;

-foreach ($list as $item) : ?>
+?>
+<?php foreach ($list as $item) : ?>
 	<li <?php if ($_SERVER['REQUEST_URI'] === JRoute::_(ContentHelperRoute::getCategoryRoute($item->id))) echo ' class="active"'; ?>> <?php $levelup = $item->level - $startLevel - 1; ?>


Remove space after <li .
Move the second <?php next line.

Quy · 2017-09-06T05:33:09Z

modules/mod_articles_category/tmpl/default.php

-						<?php if ($params->get('show_readmore')) : ?>
-							<p class="mod-articles-category-readmore">
+			<li>
+				<div class="mod-articles-category-group"><?php echo $group_name; ?></div>


Multiple lines?

Quy · 2017-09-14T00:18:00Z

modules/mod_articles_latest/tmpl/default.php

@@ -10,13 +10,13 @@
 defined('_JEXEC') or die;
 ?>


Add blank line above.

Is it possible to add lines when editing a file using Github web interface?

Quy · 2017-09-16T22:13:20Z

I have tested this item ✅ successfully on d46f2c1

Code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17851.}

first batch of module tmpl codestyle

5f1c83c

joomla-cms-bot added the PR-staging label Sep 3, 2017

Quy reviewed Sep 3, 2017

View reviewed changes

brianteeman reviewed Sep 3, 2017

View reviewed changes

zero-24 added 2 commits September 3, 2017 23:16

thanks @Quy

4d8f5b0

thanks @Quy

78bbf40

Quy reviewed Sep 4, 2017

View reviewed changes

thanks @Quy

d7dfb2d

Quy reviewed Sep 5, 2017

View reviewed changes

zero-24 added 2 commits September 5, 2017 22:41

thanks @Quy

247b813

Merge branch 'staging' of github.com:joomla/joomla-cms into modules#1

52a7769

Quy reviewed Sep 6, 2017

View reviewed changes

zero-24 and others added 4 commits September 6, 2017 18:43

thanks @Quy

b84d8ac

Merge branch 'staging' of github.com:joomla/joomla-cms into modules#1

5217f95

Merge branch 'staging' of github.com:joomla/joomla-cms into modules#1

84cd103

Indent (#34)

73abf7d

Quy reviewed Sep 14, 2017

View reviewed changes

zero-24 added 4 commits September 14, 2017 21:52

thanks @Quy

9368920

Merge branch 'modules#1' of github.com:zero-24/joomla-cms into modules#1

6b5c532

Merge branch 'staging' of github.com:joomla/joomla-cms into modules#1

8fc4061

Merge branch 'staging' of github.com:joomla/joomla-cms into modules#1

d46f2c1

zero-24 added 3 commits September 17, 2017 01:37

Merge branch 'staging' of github.com:joomla/joomla-cms into modules#1

695741f

Merge branch 'staging' of github.com:joomla/joomla-cms into modules#1

6bf4963

Merge branch 'staging' of github.com:joomla/joomla-cms into modules#1

9996980

Quy mentioned this pull request Jan 11, 2018

[CS] Code style Tabs must be used to indent lines; round 1 #19350

Merged

zero-24 closed this Jan 13, 2018

zero-24 deleted the modules#1 branch January 13, 2018 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First batch of module tmpl codestyle #17851

First batch of module tmpl codestyle #17851

zero-24 commented Sep 3, 2017

Quy Sep 3, 2017

Quy Sep 3, 2017

Quy Sep 3, 2017

Quy Sep 3, 2017

Quy Sep 3, 2017

Quy Sep 3, 2017

dgrammatiko commented Sep 3, 2017

brianteeman Sep 3, 2017

mbabker commented Sep 3, 2017

zero-24 commented Sep 3, 2017

mbabker commented Sep 3, 2017

Quy Sep 4, 2017

Quy Sep 4, 2017

Quy Sep 4, 2017

zero-24 Sep 4, 2017

Quy Sep 5, 2017

Quy Sep 5, 2017

Quy Sep 6, 2017

Quy Sep 6, 2017

Quy Sep 14, 2017

Quy commented Sep 16, 2017


		?>
		<div class="custom<?php echo $moduleclass_sfx; ?>" <?php if ($params->get('backgroundimage')) : ?> style="background-image:url(<?php echo $params->get('backgroundimage'); ?>)"<?php endif; ?> >

First batch of module tmpl codestyle #17851

First batch of module tmpl codestyle #17851

Conversation

zero-24 commented Sep 3, 2017

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrammatiko commented Sep 3, 2017

Choose a reason for hiding this comment

mbabker commented Sep 3, 2017

zero-24 commented Sep 3, 2017

mbabker commented Sep 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quy commented Sep 16, 2017