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

refactor page and media resolving, introduce ~ shortcut #3272

Merged
merged 13 commits into from Jan 2, 2022

Conversation

splitbrain
Copy link
Collaborator

This moves the resolve_id() type of functions into their own class hierarchy.

A new shortcut to be used in links is introduced. The tilde ~ can be used to reference the current page as a namespace. This is useful to dynamically create a new namespace from an existing page, effectively making that page the start page. It allows for a more dynamic growth of the wiki and makes use of the rarer used "startpage named like the namespace" method for start pages.

The existing code has not been modified, yet and continues to use the old, now deprecated methods. Some tests are still failing - before they are fixed, the expected behaviour needs to be discussed.

This moves the resolve_id() type of functions into their own class
hierarchy.

A new shortcut to be used in links is introduced. The tilde ~ can be
used to reference the current page as a namespace. This is useful to
dynamically create a new namespace from an existing page, effectively
making that page the start page. It allows for a more dynamic growth of
the wiki and makes use of the rarer used "startpage named like the
namespace" method for start pages.

The existing code has not been modified, yet and continues to use the
old, now deprecated methods. Some tests are still failing - before they
are fixed, the expected behaviour needs to be discussed.
@splitbrain
Copy link
Collaborator Author

splitbrain commented Sep 30, 2020

Things I'd like to discuss:

  • The currently failing tests: It seems like we treat an empty ID differently depending on if it's called on the homepage or not. At least that's what the comments seem to suggest. I suspect this is rather about what happens when there is no ID set in the URL rather than when a link is made somewhere. It would be easy to restore the behaviour (basically checking for an empty contextID) but I wonder if this should be treated elsewhere. Maybe we even want an UrlIdResolver to treat incoming IDs from the URL differently than when we link to an ID?
  • I'm not sure about the namespace Utils is awfully generic. Would PageUtils be any better? We're also handling media IDs here so I think not. Any better suggestions?
  • Both resolvers will always return cleaned IDs, I think that makes sense unless someone disagrees.
  • There are one or two fixmes in the code where I am not sure the code is actually doing what it is supposed to do. Would be good to have a second pair of eyes and maybe a couple of tests

This was copy'n'pasted from resolve_pageid. To correctly handle the
date_at parameter a media_exists() function had to be introduced.
Replaces the use of resolve_pageid() and resolve_mediaid() with the
proper class invocations
* master: (257 commits)
  add unit test for namespace exclusion in ft_pageLookup()
  exclude ns in pagelook ups
  translation update
  Obsolete attribute
  translation update
  translation update
  translation update
  translation update
  translation update
  Add missing `;` causing syntax error in js.php
  Run tests on PHP 8.0 now
  translation update
  translation update
  test: run test in separate process in case of error
  test: fix two tests on PHP8
  style: fix test code style
  fix method handling for RPC_CALL_ADD
  destroy the JPEGMeta object after use
  upgrade simplepie to 1.5.6
  dwpage: output meta data as JSON
  ...
* master: (37 commits)
  disable jit compiling to avoid broken pcre lib #3507
  Remove phpunit cache.
  Fix PHPUnit fatal errors compatibility with void.
  Method names with leading double underscore are reserved by PHP.
  fix named access to the sha hashing mechanisms
  adjust help text of extension cli
  move IXR XML RPC to composer dependency #1970
  translation update
  🔥 fix the calculation of file permissons
  Do not duplicate the foreach ($installed...
  translation update
  Allow installing extenions from URL via the CLI
  Add support for SHA256 encrypted passwords
  plugins/extension: Fix git recognition for plugin installations via git-submodules.
  HTTPClient: Fix missing processing of redirections with status code 303, 307, 308.
  SVG for interwiki links
  translation update
  manifest: add NOSESSION to not require auth
  add missing google interwiki link. fixes #3502
  translation update
  ...
This dosn't really change the behaviour but makes the code easier to
grasp. A simple unit test has been added.
When an empty link was given, the old implementation fell back to $ID,
this was handled incorrectly in the deprecated method.
@splitbrain

This comment has been minimized.

@splitbrain
Copy link
Collaborator Author

@Klap-in @phy25 @micgro42 or anyone else: do you have any comments on the remaining points before I merge this?

@splitbrain splitbrain added this to Triage in Release "Igor" Dec 26, 2021
@splitbrain splitbrain moved this from Triage to To Do in Release "Igor" Dec 26, 2021
@takuy
Copy link
Contributor

takuy commented Dec 27, 2021

Do you think PR can be combined with #3079? Seems like there's some overlap in regards to intent.

@Klap-in

This comment has been minimized.

@Klap-in

This comment has been minimized.

@Klap-in
Copy link
Collaborator

Klap-in commented Dec 27, 2021

  • I'm not sure about the namespace Utils is awfully generic. Would PageUtils be any better? We're also handling media IDs here so I think not. Any better suggestions?

IdUtils, NameUtils? Sounds not great as well.

@Klap-in Klap-in self-requested a review December 27, 2021 23:07
Copy link
Collaborator

@Klap-in Klap-in left a comment

Choose a reason for hiding this comment

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

Looks fine for me.

* master: (142 commits)
  authPDO: extend mysql test to ensure multiple groups are read
  update DokuWiki install URL
  update smtp plugin URL
  update flashplayer URL
  Revert "Merge pull request #3039 from takuy/video-attributes"
  Revert "fixed video attribute handling in php8"
  Revert "more php8 fixes for the video attributes"
  guard against unsert user name. fixes #3455
  remove remaining X-UA-Compatible headers. fixes #3434
  more php8 fixes for the video attributes
  fixed video attribute handling in php8
  fix test for draft file
  fix security problems in draft handling. fixes #3565
  fix handling of loading auth backend
  check CSRF token in draftdel action. fixes #3563
  ignore another PSR12 style check for now
  authplain: properly clean user names
  Removes use of deprecated create_function() in teests. Replaces them with anonymous functions. Refs #3545
  check security token on logout. fixes #3561
  create SECURITY.md fixes #3558
  ...
@splitbrain
Copy link
Collaborator Author

I moved the classes into the File namespace, I think it makes sense there.

@splitbrain splitbrain merged commit b5b12e4 into master Jan 2, 2022
Release "Igor" automation moved this from To Do to Done Jan 2, 2022
@splitbrain splitbrain deleted the refactorResolving branch January 2, 2022 10:39
@Klap-in
Copy link
Collaborator

Klap-in commented Oct 20, 2022

@splitbrain, If using [[~example]] on foo:bar, then you will see in the text ~example. For other shortcuts, e.g. [[..:page]] it shows page.
Was this intentional to keep the ~ in the simple title? My proposal is to remove it from the simple title as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants