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

[4.4] Fix newsfeed html view #43880

Merged
merged 9 commits into from
Aug 6, 2024
Merged

Conversation

muhme
Copy link
Contributor

@muhme muhme commented Aug 4, 2024

Summary of Changes

Fix newsfeed html view as down-merge from 5.1-dev. This follows #43879 and is extracted from and pre-condition for #43722 to use joomla-cypress 1.1.1 in 4.4-dev.

Testing Instructions

The warning is displayed in A) the frontenend view and B) the System Test (with a speaking checkForPhpNoticesOrWarnings). Verify the warning before patching. After patching, check that there are no more warnings.

Actual result BEFORE applying this Pull Request

A) Manual Test

  • Create a news feed in backend with Components | News Feeds | Feeds | New
  • Open /index.php?option=com_newsfeeds&view=newsfeed&id=1 (ID comes from new created news feed) shows the PHP Warning message:
screenshot

B) System Test

As preparation for the Cypress System Test install speaking checkForPhpNoticesOrWarnings in overwriting lines 74 ... 89 in node_modules/joomla-cypress/src/support.jswith the following lines:

  Cypress.Commands.add('checkForPhpNoticesOrWarnings', () => {
    cy.log('**Check for PHP notices and warnings**')

    cy.document().then((doc) => {
      const pageSource = doc.documentElement.innerHTML
      // Search for PHP problem keywords in bold style with colon and collect the found keyword and the message
      const regex = /<b>(Warning|Deprecated|Notice|Strict standards)<\/b>:(.*?)(<br|$)/
      const match = regex.exec(pageSource)
      if (match) {
        // Directly fail with the reason, the keyword found and report the PHP problem message, e.g.
        // Error: Unwanted PHP Warning: "Attempt to read property \"id\" on null in <b>/joomla-cms/components/com_newsfeeds/src/View/Category/HtmlView.php</b> on line <b>92</b>"
        throw new Error(`Unwanted PHP ${match?.[1]}: ${JSON.stringify(match?.[2])}`)
      }
    })

    cy.log('--Check for PHP notices and warnings--')
  })

Run the test specifications:

npx cypress run --spec tests/System/integration/site/components/com_newsfeed/NewsFeed.cy.js

Fails with:

Running:  NewsFeed.cy.js                                                                  (1 of 1)

  Test in frontend that the newsfeeds details view
    ✓ can display a feed in a menu item from joomla.org (530ms)
    1) "after each" hook for "can display a feed without a menu item from joomla.org"

  1 passing (1s)
  1 failing

  1) Test in frontend that the newsfeeds details view
       "after each" hook for "can display a feed without a menu item from joomla.org":
     Error: Unwanted PHP Warning: "  Attempt to read property \"id\" on null in <b>/Users/hlu/Desktop/no_backup/joomla-cms/components/com_newsfeeds/src/View/Newsfeed/HtmlView.php</b> on line <b>264</b>"

💡 With overwritten speaking checkForPhpNoticesOrWarnings, if you run the entire system tests, there are four failures before this PR and only three after. The remaining three errors are fixed by #43879 and #43881.

Expected result AFTER applying this Pull Request

No warnings in A) frontend view or B) System Test.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

Up-Merging

@LadySolveig @bembelimen there is no up-merge required as it is only a down-merge from 5.1-dev

muhme added 9 commits June 7, 2024 06:47
Consider file permissions when writing configuration in system tests …
merge joomla/joomla-cms branch 4.4-dev
merge head-repository joomla/joomla-cms
merge head repository joomla/joomla-cms
merge from head repository
@alikon
Copy link
Contributor

alikon commented Aug 5, 2024

I have tested this item ✅ successfully on 68097e9


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

@fgsw
Copy link

fgsw commented Aug 6, 2024

I have tested this item ✅ successfully on 68097e9

Manual Test (A) only.


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

@laoneo laoneo removed the PR-4.4-dev label Aug 6, 2024
@laoneo
Copy link
Member

laoneo commented Aug 6, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 6, 2024
@laoneo laoneo merged commit 897f05b into joomla:4.4-dev Aug 6, 2024
4 of 5 checks passed
@laoneo
Copy link
Member

laoneo commented Aug 6, 2024

Thanks!

@joomla-cms-bot joomla-cms-bot added PR-4.4-dev and removed RTC This Pull Request is Ready To Commit labels Aug 6, 2024
@laoneo laoneo added this to the Joomla! 4.4.7 milestone Aug 6, 2024
@muhme muhme deleted the fix-newsfeed-htmlview branch August 6, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants