Skip to content

Fix metadata rendering problem #609

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

Closed
wants to merge 6 commits into from

Conversation

alexdraconian
Copy link
Contributor

Fix various issues with metadata render.

Current code has following problems, with metadata rendering.

  • List and cloud internal links are not be rendered to metadata.
  • Table filters ARE NOT be correctly applied to metadata rendering. This means, EVERY pages will be added as internal link to metadata, regardless of table filter options. As a result, backlinks of page will contain non-related pages.
  • Dynamic filters and limit parameter ARE applied to metadata rendering. This means every time dynamic filter is applied or user visit another page of table, backlinks will also be changed dynamically. This will delete related pages from backlink.

This pull request resolves above problems with following solution:

  • Enable list and cloud metadata rendering
  • In metadata rendering, dynamic filters and limit parameter will be ignored. To achieve this, I edited code so that syntax can be rendered twice; metadata and xhtml.

I tested this code with my personal wiki, which has 1,200 pages and a lot of struct aggregation. But if I missed something, please let me know.

Fix various issues with metadata render.
Solved problem that certain schema data is not rendered properly.
*/
public function __construct($config)
public function __construct($config, $renderMetadata=false)
Copy link
Member

Choose a reason for hiding this comment

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

Minor thing: I would rename the parameter to be independent from it's current usage and align it with what it actually does. Eg. $applyDynamicFilters or something similar

Copy link
Member

Choose a reason for hiding this comment

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

This still seems to be unsolved. The parameter is has nothing to do with metadata rendering. It defines if dynamic parameters should be applied or not (which coincidentally should not happen when rendering metadata).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code of __construct not only blocks dynamic filter, but also blocks limit parameter already defined with aggregation syntax. Is ``$applyDynamicFilters'' still valid in this case?

Copy link
Member

@splitbrain splitbrain left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this PR. It looks good already. I would like to see some unit tests for the new behaviour in SearchConfig.

@splitbrain
Copy link
Member

There's also something wrong in the SearchConfig class (see broken tests). Probably a bracket mismatch somewhere?

Fix wrongly inserted code.
Additional fix for list and cloud metadata issues.
Allow metadata rendering from other pages using "p_get_metadata()", by changing "$ID$" and current pagename resolution method. (e.x.orphanswanted plugin)
@splitbrain
Copy link
Member

Any chance you could add test cases?

@alexdraconian
Copy link
Contributor Author

alexdraconian commented May 25, 2022

My apologize, I didn't write unit test before, so It may take some time.

@alexdraconian
Copy link
Contributor Author

I see there're a lot of refactoring of filter algotithm is going on; It seems I have to overhaul everything in here after refactoring is done?

@alexdraconian alexdraconian deleted the metadata-fix branch August 7, 2023 12:35
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.

2 participants