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

WIP: Project id by query string #1851

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

starlocke
Copy link

https://www.mantisbt.org/bugs/view.php?id=9193 ("Selected project" state should be client-side not server-side)

@starlocke starlocke marked this pull request as draft September 13, 2022 02:34
@starlocke
Copy link
Author

@dregad - pinging you on this PR as a reminder that it is awaiting review/feedback.

@vboctor
Copy link
Member

vboctor commented Mar 29, 2023

Thanks @starlocke for working on this. It is a change that touches a lot of areas. I had a look and have the following comments:

  1. It feels like we can significantly change the places we touch if we leverage some of the existing APIs. For example: html_meta_redirect(), print_successful_redirect_to_bug(), print_button(), print_form_button(), etc.
  2. Doing the above, will also reduces the number of places where plugins will be broker, assuming they use some of these functions.
  3. As discussed in the bug, we should continue to keep the current approach of remembering the currently selected project as a backup.
  4. For forms, we should consider to add a form_start() / form_end(). We could have such methods handle form name, security token generation (when name is supplied), and URL redirect.
  5. I wonder if we should consider adding a url_api.php to core that has a URL class that can build a URL from a path, array of query parameters and override project. Then use that and accept that or a string in the APIs that handle URLs. Pages typically will just create a URL, set relative path, array of params and pass it on.
  6. I haven't looked at the dependency on URL normalizer, but if with the above, it is not critical, I would skip it. Specially if it pulls in a bunch of dependencies (haven't looked at that project).
  7. We may need to squash these changes and put them on top of master rather than merge. But we can worry about this later.
  8. Project affinity indicator is fine for your development / testing, but I don't expect to have this merged.

Hope this helps. Thanks again. Will add more as the PR progresses.

@vboctor
Copy link
Member

vboctor commented Mar 29, 2023

You may want to consider working on the core concept and few pages (3-4) and once we settle on the approach and have it working there, then we can expand to all pages. I would look at View Issues, Issue CRUD pages as an example. Otherwise, every iteration will require a lot of changes across all pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants