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

Remove filter="url" for wrapper iframe #37547

Merged
merged 6 commits into from
Sep 4, 2023

Conversation

chmst
Copy link
Contributor

@chmst chmst commented Apr 12, 2022

Pull Request for Issue #37545 .

Summary of Changes

Remove filter="url" for wrapper iframe url.

Testing Instructions

see issue #37545

Actual result BEFORE applying this Pull Request

Url without http(s):// gets prefix http:// automatically

Expected result AFTER applying this Pull Request

Url without prefix is possible.

Make sure that the url then is correct in frontend.

Documentation Changes Required

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 106b730

The autoadd functionality is now never available


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

@chmst
Copy link
Contributor Author

chmst commented Apr 12, 2022

Removing also validate - enables relative urls as mentioned in #33235

@chmst
Copy link
Contributor Author

chmst commented Apr 12, 2022

The autoadd function works only in frontend, not in backend. You can see the code in components\com_wrapper\src\View\Wrapper\HtmlView.php. In my opinion this is wrong in the view but this is not in scope here

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 106b730

sorry - misunderstood


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

@Quy
Copy link
Contributor

Quy commented Apr 12, 2022

This reverts issue #32245 and PR #32342.

@bayareajenn
Copy link

@Quy is correct. It reverts those other issue/PRs. I can never find this stuff so thank you for finding it.
And thank you @chmst for writing the PR. Whilst I'd love to have it back to the way it was, if it isn't going to, we can write it into our custom extension.
To me there's no point is having an option of "No" if it doesn't actually do what it says. We use a shortcode to a different server in that field in our custom extension. In working on getting our J3 extension ready for J4 is when I found this "bug" which doesn't appear to be a bug but rather other people deciding no one uses it in a different way. Bummer.

So someone let me know if I should test and then the other PR would be reverted or if we close this and I deal with it a different way, k?

Thanks for testing @brianteeman

@chmst
Copy link
Contributor Author

chmst commented Apr 12, 2022

Thanks @Quy. If this enforced http//: is intended, then the the code in components\com_wrapper\src\View\Wrapper\HtmlView.php is wrong. I never use this and so have no opinion

@bayareajenn
Copy link

I have tested this item ✅ successfully on 106b730

Works great and how it did before #32245 and PR #32342.

Let's see what happens now.


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

@zero-24
Copy link
Contributor

zero-24 commented Apr 13, 2022

hmm i dont think we should remove the validation here. Better we should create a filter that also allows relative URLs where the question should be asked first why should we iframe an internal URL in the first place?

And by the intial report the 'issue' is that the protocol has been added? why is that an issue isnt adding the protocol not something thats a good thing?

maybe I'm missing something here?

@bayareajenn
Copy link

@zero-24 At my company, we aren't using an internal URL. We're using a shortcode off to a different server. Our current extension adds the https so in the menu item we kept it turned off. We can't go adding http(s) twice now, can we?! So two choices: the option is added back into Joomla or my company changes our extension to NOT have the http(s) to make up for the change and maybe not be able to use the shortcode anymore since I don't think we can have Joomla detect the "s" on a shortcode.

Simply said, it was something we used. I didn't know it had disappeared in J4. I thought it was a bug.

The fact that the setting/parameter Auto Add can be set to "No" but doesn't obey "No" should be addressed though. Either allow it to be turned off for reals, or remove the Auto Add parameter completely since it doesn't work. It adds http when it is set to No.


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

@zero-24
Copy link
Contributor

zero-24 commented Apr 13, 2022

We're using a shortcode off to a different server.

What is a shortcode in this context? In the past it was just a "you can add here anything" field now we make sure its an URL including the protocol.

@frostmakk
Copy link
Contributor

From the iframe wrapper menu's help screen:

image

The question is not why should we allow a relative path. The question is why did we remove a valid and documented option that people are using?

@zero-24
Copy link
Contributor

zero-24 commented Apr 13, 2022

The question is not why should we allow a relative path. The question is why did we remove a valid and documented option that people are using?

I still have the question why we need to use relative paths in the first place but when its documented that way an filter that allows relative paths should be created and we should not just remove the current url filter

@zero-24
Copy link
Contributor

zero-24 commented Apr 13, 2022

In the best case there should just be an option within the current URL filter like "allowRelative=true" and in case its not set its not allowing relative URLs while this option will be set here ;)

@frostmakk
Copy link
Contributor

Why not simply switch the validation filter on/off with the existing Auto Add option button ?

@zero-24
Copy link
Contributor

zero-24 commented Apr 13, 2022

Why not simply switch the validation filter on/off with the existing Auto Add option button ?

That would mean the validation rule has to check the option of an specific backend component which is not a good idea and auto Add has nothing to do with the validation but is adding stuff the the when the user does not add it.

@bayareajenn
Copy link

bayareajenn commented Apr 13, 2022

What is a shortcode in this context? In the past it was just a "you can add here anything" field now we make sure its an URL including the protocol.

We have a lot of sites. Instead of having to put the server name in each site (different for every instance), we use a shortcode - imagine it's something like _SERVER_ and it pulls from the parameters of the custom extension which does have the server name spelled out. It makes it easier for our people who build the sites (globally).

This is the second thing to be removed from the iframe menu item but the documentation says it exists #37544.

In the case of this issue, I can work around it if I have to but the fact remains, there's an Auto Add option toggle that should allow me to turn it off and I can't. So is it a bug? Or is it just other people deciding what's good for me to put in the URL field? OR should the Auto Add parameter also be removed since it's a pointless toggle?

@chmst chmst added the RMDQ ReleaseManagerDecisionQueue label Apr 13, 2022
@Pallieguy
Copy link

We have custom scripts/tools that are developed on a development environment before being moved to production, using a relative path allows us to copy and paste everything between our production and development environments instead of manually setting them.

While not a lot of work, it's unnecessary work and one more thing for someone to miss when trying to code/debug

@joomla-cms-bot joomla-cms-bot removed the RMDQ ReleaseManagerDecisionQueue label Apr 21, 2022
@RickR2H
Copy link
Member

RickR2H commented Apr 21, 2022

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 21, 2022
@zero-24
Copy link
Contributor

zero-24 commented Apr 21, 2022

@RickR2H please take off the RTC label as there is still an open discussion how to solve this issue.

@brianteeman
Copy link
Contributor

and please put the RLDQ tag back

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 21, 2022
@bayareajenn
Copy link

@bembelimen and @zero-24 any updates on this one?

@mpradel
Copy link

mpradel commented May 27, 2022

I have tested this item ✅ successfully on 106b730


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

@HLeithner
Copy link
Member

This pull requests has automatically rebased to 4.2-dev.

@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:05
@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@HLeithner HLeithner removed the psr12 label Oct 23, 2022
@chmst chmst added the Small A PR which only has a small change label Feb 25, 2023
@Hackwar Hackwar added the bug label Apr 6, 2023
@richard67 richard67 mentioned this pull request Apr 27, 2023
@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev May 2, 2023 16:30
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.3-dev.

@brianteeman
Copy link
Contributor

This has been waitinf ro a release lead to make a decision for over 1 year :(

@bayareajenn
Copy link

bayareajenn commented Jul 12, 2023 via email

@TLWebdesign
Copy link

I've tested this and noticed that when no schema was added to the url it still added a single slash in front of the url in the generated html. When i looked at the dom it looked like this: src="/domain.nl" while the url field only contains domain.nl


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

@obuisard obuisard changed the base branch from 4.3-dev to 4.4-dev August 26, 2023 16:14
@MacJoom
Copy link
Contributor

MacJoom commented Aug 31, 2023

I've tested this and noticed that when no schema was added to the url it still added a single slash in front of the url in the generated html. When i looked at the dom it looked like this: src="/domain.nl" while the url field only contains domain.nl

@TLWebdesign - thanks for testing - is this a problem within this PR or another issue?

@MacJoom MacJoom self-assigned this Aug 31, 2023
@MacJoom MacJoom merged commit c71964d into joomla:4.4-dev Sep 4, 2023
3 checks passed
@MacJoom MacJoom added this to the Joomla! 4.4.0 milestone Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PR-4.4-dev RMDQ ReleaseManagerDecisionQueue Small A PR which only has a small change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet