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.

Sign up for GitHub

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

Jump to bottom

[ticket/13934] Add enctype clause for profile fields #3733

Merged
merged 1 commit into from Oct 8, 2015
Merged

[ticket/13934] Add enctype clause for profile fields #3733

merged 1 commit into from Oct 8, 2015

Conversation

javiexin
Copy link
Contributor

Adds a template variable (the same) in all places where profile fields
may need an enctype clause in the corresponding forms.

PHPBB3-13934

@javiexin
[ticket/13934] Add enctype clause for profile fields
80fee74 
Adds a template variable (the same) in all places where profile fields
may need an enctype clause in the corresponding forms.

PHPBB3-13934
CHItA
CHItA reviewed Jun 30, 2015
View reviewed changes
phpBB/adm/style/acp_profile.html
@@ -17,7 +17,7 @@
</div>
<!-- ENDIF -->

<form id="add_profile_field" method="post" action="{U_ACTION}">
<form id="add_profile_field" method="post" action="{U_ACTION}"{S_FORM_ENCTYPE}>
Copy link
Member

Choose a reason for hiding this comment

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

Please use <!-- IF ... --> {S_FORM_ENCTYPE}<!-- ENDIF --> all the places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? This is not the way it is done elsewhere in phpbb...

Copy link
Member

Choose a reason for hiding this comment

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

Because it is a clearer and better way of doing this. You should be able to use <!-- IF S_FORM_ENCTYPE is defined and S_FORM_ENCTYPE is not empty --> {S_FORM_ENCTYPE}<!-- ENDIF -->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Examples of usage in other areas of phpbb. And there are a lot more, and none like what you propose.
I don't understand why this would be any different. I prefer consistency across the phpbb software.
https://github.com/phpbb/phpbb/blob/3.1.x/phpBB/styles/prosilver/template/ucp_profile_profile_info.html#L3
https://github.com/phpbb/phpbb/blob/3.1.x/phpBB/styles/prosilver/template/posting_layout.html#L24

Copy link
Member

Choose a reason for hiding this comment

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

Please add the enctype attribute as well to the template. If you are bothered by the inconsistency of the template files, please feel free to send a patch for those as well.

Copy link
Member

Choose a reason for hiding this comment

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

that's what you wrote, though... ;)

Copy link
Member

Choose a reason for hiding this comment

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

needs a space after the quote too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not if used as in other places, see links above. The space is supposed to be included in the template var. But it may be safer if included again here...

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont really understand the problem. If something is undefined, using it like above will just be fine. It will be false or whatever and not causing any output. So it has the same result.

Copy link
Member

Choose a reason for hiding this comment

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

I just hate HTML in the backend, it should not have got there in the first place.

@CHItA CHItA added this to the 3.1.6 milestone Jun 30, 2015
iMattPro
iMattPro reviewed Jul 1, 2015
View reviewed changes
phpBB/adm/style/acp_users_profile.html
@@ -1,4 +1,4 @@
<form id="user_profile" method="post" action="{U_ACTION}">
<form id="user_profile" method="post" action="{U_ACTION}"{S_FORM_ENCTYPE}>
Copy link
Member

Choose a reason for hiding this comment

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

space?

@javiexin
Copy link
Contributor Author

javiexin commented Jul 1, 2015

I have been looking at current phpbb 3.1.5, to identify all CURRENT uses of enctype, and these are my findings:
ACP templates:

  • 2 templates have enctype clause hardcoded (no template variable)
  • 1 template has enctype clause surrounded by ; new in 3.1, and using a new different template variable S_CAN_UPLOAD

Prosilver templates:

  • 19 templates have enctype template variable S_FORM_ENCTYPE as used in this PR, without and without space between the ACTION template variable and this

PHP code:

  • 5 php files fill in the same template variable (S_FORM_ENCTYPE), and in all cases, the value includes the starting space

What I have not been able to see is the amount of extension that make use of such variables; I don't know if it is difficult for you to run such a query, at least among the validated extensions.

With all of this, my proposal and preferences are as follows:

  1. Continue with my original proposal, for consistency with current uses within phpbb software
  2. If that is unacceptable, I would be OK with hardcoding the ACP templates maintaining the prosilver template as proposed, although I prefer the former
  3. I would not accept the use of conditional for prosilver templates unless the already existing 19 cases are updated as well.

If you all agree that introducing the initial space in the templates and removing it from the PHP files is better, someone please open a separate ticket, and I will be happy to provide a PR for that, and will update this to include the space here, as I agree this could be error prone, and even, source of potential security exploits (by appending text to the URL that is in front), and no extension should be using it anyway (at least not cleanly).

If this needs to be changed further, like updating existing uses either in ACP or in prosilve to use conditionals surrounding them, I would suggest that it goes through an RFP process in Area51 AND Extension Writers forum, as this will definitely break a number of existing or in development extensions. It might not be a bad idea, but we need to know the impact of such a change before implementing it, as it is heavily non-backwards compatible.

Please, advise on what to do next.

@nickvergessen
Copy link
Contributor

The patch looks fine to me, when the variables are properly set in the corresponding php files.

@Nicofuma Nicofuma modified the milestones: 3.1.6, 3.1.7 Sep 5, 2015
@javiexin
Copy link
Contributor Author

javiexin commented Oct 4, 2015

Could anyone please review this further, so that it can be either accepted or rejected?
Thanks in advance.

@Nicofuma Nicofuma merged commit 80fee74 into phpbb:3.1.x Oct 8, 2015
Nicofuma pushed a commit that referenced this pull request Oct 8, 2015
Merge pull request #3733 from javiexin/ticket/13934
5b7b089 
[ticket/13934] Add enctype clause for profile fields

* javiexin/ticket/13934:
  [ticket/13934] Add enctype clause for profile fields
@Nicofuma
Copy link
Member

Nicofuma commented Oct 8, 2015

thanks for your contribution

@javiexin javiexin deleted the ticket/13934 branch October 26, 2016 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
3.1 (Ascraeus) Styles 🎨
Projects
None yet
Milestone
3.1.7
5 participants
@javiexin @nickvergessen @Nicofuma @iMattPro @CHItA