Skip to content

Rewrite PHP opening tags#7939

Merged
Alkarex merged 1 commit intoFreshRSS:edgefrom
aledeg:refactor/rewrite-php-tags
Sep 10, 2025
Merged

Rewrite PHP opening tags#7939
Alkarex merged 1 commit intoFreshRSS:edgefrom
aledeg:refactor/rewrite-php-tags

Conversation

@aledeg
Copy link
Member

@aledeg aledeg commented Sep 10, 2025

This allows to remove the use of echo and be consistent through out the file. Some empty PHP tags where removed as well.

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard)
  • documentation updated

Additional information can be found in the documentation.

This allows to remove the use of `echo` and be consistent through out the file.
Some empty PHP tags where removed as well.
@aledeg aledeg requested a review from Alkarex September 10, 2025 17:17
@aledeg
Copy link
Member Author

aledeg commented Sep 10, 2025

If I am not mistaken, I think some PHP tags were useless.

@Frenzie
Copy link
Member

Frenzie commented Sep 10, 2025

It does seem strange that there's a seemingly random echo in between the <?=.

If I am not mistaken, I think some PHP tags were useless.

Slightly mistaken I believe. The line-break and indentation are hidden in the output that way without needing post-processing.

@aledeg
Copy link
Member Author

aledeg commented Sep 10, 2025

It does seem strange that there's a seemingly random echo in between the <?=.

I am not sure to understand what you mean. I replaced <?php echo to use <?= instead.

If I am not mistaken, I think some PHP tags were useless.

Slightly mistaken I believe. The line-break and indentation are hidden in the output that way without needing post-processing.

Still not sure to understand. No matter how many line breaks, white spaces or tabs you have, the output will be the same in the browser.

@Frenzie
Copy link
Member

Frenzie commented Sep 10, 2025

I mean the prior situation with the echo seemed surprising. :-)

Still not sure to understand.

Whoever wrote it didn't want it to be in the HTML at all. Spaces are collapsed into a single space but that's not the same thing as no space at all.

@Alkarex Alkarex added this to the 1.27.1 milestone Sep 10, 2025
@aledeg
Copy link
Member Author

aledeg commented Sep 10, 2025

I see.
But with the modification, there is still a space between HTML tags since they are on different lines.

@Alkarex
Copy link
Member

Alkarex commented Sep 10, 2025

The echo statements were forgotten when the move to <?= ... ?> was done, so good fix.

The other changes are fine as well. In code sections used many times (such as in article headers), I am sometimes stripping the white-space to reduce output size, but here, this code is outputted only once, so it does not matter much.

The new PR produces this kind of white-space:

<div class="day" id="day_before_yesterday">Reçu avant avant-hier				<span class="name"></span>
			</div>

@Alkarex Alkarex merged commit c2009b5 into FreshRSS:edge Sep 10, 2025
1 check passed
@aledeg
Copy link
Member Author

aledeg commented Sep 10, 2025

I understand the idea behind the empty tags now. Maybe we could have some kind of post-processor to remove the overhead of white spaces. But then we have an overhead of processing time.

When using template engines (blade, twig, ...), they have options to remove unnecessary white spaces. Maybe we should add that as well.

@aledeg aledeg deleted the refactor/rewrite-php-tags branch September 10, 2025 19:59
@math-GH
Copy link
Contributor

math-GH commented Sep 10, 2025

I am late to the party and want to second @Frenzie

?><span class="name"><?= FreshRSS_Context::$name ?></span><?php
?></div><?php
?><div class="day" id="day_today"><?= _t('index.feed.received.today') ?>
<span class="date"> — <?= timestamptodate(time(), false) ?></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

now there is more than 1 space before the

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it is a good point to remove the and replace it with CSS because this character is used for styling here. CSS would be the better solution IMHO

Copy link
Member

Choose a reason for hiding this comment

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

PR welcome if CSS is better. Is there any regression here? If so, can be fixed in #7941

Copy link
Member

Choose a reason for hiding this comment

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

Like I said, no space is different than a space. Any number of tabs, spaces and newlines are collapsed into a space but that's fundamentally distinct from no space.

@Alkarex
Copy link
Member

Alkarex commented Sep 10, 2025

Maybe we could have some kind of post-processor to remove the overhead of white spaces. But then we have an overhead of processing time.

Yes, I would not be in favour of using any other layer of processing. There are only a few places where I think we should make an effort with white-space and that can be done with native syntax easily enough.

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Sep 10, 2025
@Alkarex
Copy link
Member

Alkarex commented Sep 10, 2025

A little follow-up #7941

Alkarex added a commit that referenced this pull request Sep 11, 2025
* Minor update syntax echo
Follow-up of #7939

* Fix layout
Whitespace optimisation needed to avoid style glitch
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.

4 participants