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

Optimize path_admin_form(): Avoid calling url() multiple times. #4963

Closed
klonos opened this issue Feb 22, 2021 · 7 comments · Fixed by backdrop/backdrop#3533
Closed

Optimize path_admin_form(): Avoid calling url() multiple times. #4963

klonos opened this issue Feb 22, 2021 · 7 comments · Fixed by backdrop/backdrop#3533

Comments

@klonos
Copy link
Member

klonos commented Feb 22, 2021

This came up while reviewing backdrop/backdrop#3522

See backdrop/backdrop#3522 (comment)

Here, we could do something like:

  $field_prefix = url(NULL, array('absolute' => TRUE)) . ($site_config->get('clean_url') ? '' : '?q=');

...and then reuse:

  '#field_prefix' => $field_prefix,

It would save us 2 calls to url() 😉 ...but perhaps scope creep for this PR.

@klonos
Copy link
Member Author

klonos commented Feb 22, 2021

PR: backdrop/backdrop#3533

@indigoxela
Copy link
Member

@klonos your PR totally makes sense, but as the same lines already change in the related PR, you'll run into a conflict.

I suppose, one of these PRs has to wait for the other one to get merged.

@klonos
Copy link
Member Author

klonos commented Feb 26, 2021

I suppose, one of these PRs has to wait for the other one to get merged.

Yup. I only created this PR because as small a change as this may be, it still was "scope creep" for the other issue/PR. Happy to wait 🙂

@indigoxela
Copy link
Member

As expected - the other issue got merged, so this one now has conflicts. The PR needs a tiny update.

@ghost
Copy link

ghost commented Jun 18, 2022

I've fixed the merge conflicts in the PR. Ready for review/testing.

@indigoxela
Copy link
Member

@BWPanda many thanks for updating this mini-improvement. It still works and the code still makes sense. RTBC 👍

@quicksketch
Copy link
Member

Thanks folks! I've merged backdrop/backdrop#3533 into 1.x and 1.21.x.

@jenlampton jenlampton changed the title Slightly optimize path_admin_form() so that it doesn't call url() multiple times Fixed: Optimize path_admin_form(): Avoid calling url() multiple times. Jul 20, 2022
@jenlampton jenlampton changed the title Fixed: Optimize path_admin_form(): Avoid calling url() multiple times. Optimize path_admin_form(): Avoid calling url() multiple times. Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants