Permalink
Browse files

Fix "<onchange>" tag processing in package XML handling

  • Loading branch information...
sbeaver-netgate committed Dec 21, 2016
1 parent caee8fc commit a038b8166a332579112f52639fd5ac03b5f76565
Showing with 2 additions and 2 deletions.
  1. +2 −2 src/usr/local/www/pkg_edit.php
@@ -826,7 +826,7 @@ function parse_package_templates() {
$items = array($value);
}
$onchange = (isset($pkga['onchange']) ? "onchange=\"{$pkga['onchange']}\"" : '');
$onchange = (isset($pkga['onchange']) ? "{$pkga['onchange']}" : '');
foreach ($pkga['options']['option'] as $opt) {
$optionlist[$opt['value']] = $opt['name'];
@@ -877,7 +877,7 @@ function parse_package_templates() {
$items = array($value);
}
$onchange = (isset($pkga['onchange']) ? "onchange=\"{$pkga['onchange']}\"" : '');
$onchange = (isset($pkga['onchange']) ? "{$pkga['onchange']}" : '');
$source_url = $pkga['source'];
eval("\$pkg_source_txt = &$source_url;");

32 comments on commit a038b81

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta

lgcosta Dec 28, 2016

Contributor

this change is causing a bug in the final code with last package from Squid, see that the syntax has been duplicated
captura de tela_2016 12 28_11 19 02

Contributor

lgcosta replied Dec 28, 2016

this change is causing a bug in the final code with last package from Squid, see that the syntax has been duplicated
captura de tela_2016 12 28_11 19 02

@sbeaver-netgate

This comment has been minimized.

Show comment
Hide comment
@sbeaver-netgate

sbeaver-netgate Dec 28, 2016

Contributor

You have highlighted the original bug, not the fix.

Contributor

sbeaver-netgate replied Dec 28, 2016

You have highlighted the original bug, not the fix.

@LFCavalcanti

This comment has been minimized.

Show comment
Hide comment
@LFCavalcanti

LFCavalcanti Dec 28, 2016

@sbeaver-netgate

Look at the image, the code is duplicated, calling the same functions, generating syntax error when trying to set any auth method on Squid.

Just tested on 2.3.2_p1, with latest Squid.

LFCavalcanti replied Dec 28, 2016

@sbeaver-netgate

Look at the image, the code is duplicated, calling the same functions, generating syntax error when trying to set any auth method on Squid.

Just tested on 2.3.2_p1, with latest Squid.

@doktornotor

This comment has been minimized.

Show comment
Hide comment
@doktornotor

doktornotor Dec 28, 2016

Contributor

You CANNOT test this on 2.3.2 because the fixes are NOT there.

Contributor

doktornotor replied Dec 28, 2016

You CANNOT test this on 2.3.2 because the fixes are NOT there.

@sbeaver-netgate

This comment has been minimized.

Show comment
Hide comment
@sbeaver-netgate

sbeaver-netgate Dec 28, 2016

Contributor
Contributor

sbeaver-netgate replied Dec 28, 2016

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta

lgcosta Dec 28, 2016

Contributor

Yes, i understood. But this killed the new installations of the Squid package in 2.3.2

Contributor

lgcosta replied Dec 28, 2016

Yes, i understood. But this killed the new installations of the Squid package in 2.3.2

@sbeaver-netgate

This comment has been minimized.

Show comment
Hide comment
@sbeaver-netgate

sbeaver-netgate Dec 28, 2016

Contributor

How is it killed? The javascript did not work before and it will not work now in 2.3.2

Contributor

sbeaver-netgate replied Dec 28, 2016

How is it killed? The javascript did not work before and it will not work now in 2.3.2

@LFCavalcanti

This comment has been minimized.

Show comment
Hide comment
@LFCavalcanti

LFCavalcanti Dec 28, 2016

@sbeaver-netgate

I can confirm with 100% certainty that before this patch selecting authentication on Squid was working, either we are talking about different features or some miscommunication happened concerning the issue.

LFCavalcanti replied Dec 28, 2016

@sbeaver-netgate

I can confirm with 100% certainty that before this patch selecting authentication on Squid was working, either we are talking about different features or some miscommunication happened concerning the issue.

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta

lgcosta Dec 28, 2016

Contributor

@sbeaver-netgate I am now with one install pfSense, version 2.3.2 and installed the latest version of the Squid package.
I could not use the authentication option, so seeing in the code, i saw this duplicate syntax

Contributor

lgcosta replied Dec 28, 2016

@sbeaver-netgate I am now with one install pfSense, version 2.3.2 and installed the latest version of the Squid package.
I could not use the authentication option, so seeing in the code, i saw this duplicate syntax

@sbeaver-netgate

This comment has been minimized.

Show comment
Hide comment
@sbeaver-netgate

sbeaver-netgate Dec 28, 2016

Contributor

You saw that the duplicate syntax had been removed.

Contributor

sbeaver-netgate replied Dec 28, 2016

You saw that the duplicate syntax had been removed.

@LFCavalcanti

This comment has been minimized.

Show comment
Hide comment
@LFCavalcanti

LFCavalcanti Dec 28, 2016

I have, in production, a 2.3.2_p1 with Squid 0.4.23_2, working with NT Domain authentication.

Has this been merged onto 2_3_ ?

LFCavalcanti replied Dec 28, 2016

I have, in production, a 2.3.2_p1 with Squid 0.4.23_2, working with NT Domain authentication.

Has this been merged onto 2_3_ ?

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta

lgcosta Dec 28, 2016

Contributor
[2.3.2-RELEASE][root@pfSense.lab.local]/usr/local/pkg: cat /etc/version
2.3.2-RELEASE
[2.3.2-RELEASE][root@pfSense.lab.local]/usr/local/pkg: cat /etc/version.patch
1
[2.3.2-RELEASE][root@pfSense.lab.local]/usr/local/pkg: pkg info | grep pkg-squid
pfSense-pkg-squid-0.4.29       pfSense package squid
Contributor

lgcosta replied Dec 28, 2016

[2.3.2-RELEASE][root@pfSense.lab.local]/usr/local/pkg: cat /etc/version
2.3.2-RELEASE
[2.3.2-RELEASE][root@pfSense.lab.local]/usr/local/pkg: cat /etc/version.patch
1
[2.3.2-RELEASE][root@pfSense.lab.local]/usr/local/pkg: pkg info | grep pkg-squid
pfSense-pkg-squid-0.4.29       pfSense package squid
@doktornotor

This comment has been minimized.

Show comment
Hide comment
@doktornotor

doktornotor Dec 28, 2016

Contributor

Sir. You have been told about 4 times by now that NONE of the fixes are available in stuff released months ago. There is also no NT Domain authentication in the current Squid package (0.4.29) available in 2.3.3+, because it has never worked. So, the JS has NOT ruined your Squid, it is a purely cosmetic issue that makes configuration less intuitive due to potentially filling in irrelevant stuff. The NT Domain auth never worked, since 2.3 release. Use LDAP.

Contributor

doktornotor replied Dec 28, 2016

Sir. You have been told about 4 times by now that NONE of the fixes are available in stuff released months ago. There is also no NT Domain authentication in the current Squid package (0.4.29) available in 2.3.3+, because it has never worked. So, the JS has NOT ruined your Squid, it is a purely cosmetic issue that makes configuration less intuitive due to potentially filling in irrelevant stuff. The NT Domain auth never worked, since 2.3 release. Use LDAP.

@LFCavalcanti

This comment has been minimized.

Show comment
Hide comment
@LFCavalcanti

LFCavalcanti Dec 28, 2016

@doktornotor , you're right, LDAP... not NT Domain. I stand corrected on that.

Then why the latest Squid version for 2.3.2 produces and error when trying to select and authentication method? This fix is not merged with that branch?

Honest question, no need to be angry about it.

LFCavalcanti replied Dec 28, 2016

@doktornotor , you're right, LDAP... not NT Domain. I stand corrected on that.

Then why the latest Squid version for 2.3.2 produces and error when trying to select and authentication method? This fix is not merged with that branch?

Honest question, no need to be angry about it.

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta

lgcosta Dec 28, 2016

Contributor

If you can reproduce, just install a version of pfsense 2.3.2_1 and install the squid package. Ok, check the exit code for squid_auth.xml

Contributor

lgcosta replied Dec 28, 2016

If you can reproduce, just install a version of pfsense 2.3.2_1 and install the squid package. Ok, check the exit code for squid_auth.xml

@doktornotor

This comment has been minimized.

Show comment
Hide comment
@doktornotor

doktornotor Dec 28, 2016

Contributor

No, this fix is NOT merged with that branch. For the 5th time. (And there's never been any error when trying to select anything on any browser I tried - FF, Chrome, IE.)

Contributor

doktornotor replied Dec 28, 2016

No, this fix is NOT merged with that branch. For the 5th time. (And there's never been any error when trying to select anything on any browser I tried - FF, Chrome, IE.)

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta

lgcosta Dec 28, 2016

Contributor

Guys, see the screencast about this issue:
https://www.youtube.com/watch?v=AgvY_SDX_HY

Contributor

lgcosta replied Dec 28, 2016

Guys, see the screencast about this issue:
https://www.youtube.com/watch?v=AgvY_SDX_HY

@doktornotor

This comment has been minimized.

Show comment
Hide comment
@doktornotor

doktornotor Dec 28, 2016

Contributor

Sigh. The issue is FIXED in 2.3.3/0.4.29. If you are having an issue that noone can reproduce, then use that and move on.

So far I've received 12 absolutely pointless email notifications from GitHub and Redmine about the comments and I'm starting to get rather irritated by the amount of noise.

Contributor

doktornotor replied Dec 28, 2016

Sigh. The issue is FIXED in 2.3.3/0.4.29. If you are having an issue that noone can reproduce, then use that and move on.

So far I've received 12 absolutely pointless email notifications from GitHub and Redmine about the comments and I'm starting to get rather irritated by the amount of noise.

@rbgarga

This comment has been minimized.

Show comment
Hide comment
@rbgarga

rbgarga Dec 28, 2016

Member

It was merged to RELENG_2_3_2 and it is my fault. I'll revert all these changes and let you know. Sorry.

Member

rbgarga replied Dec 28, 2016

It was merged to RELENG_2_3_2 and it is my fault. I'll revert all these changes and let you know. Sorry.

@sbeaver-netgate

This comment has been minimized.

Show comment
Hide comment
@sbeaver-netgate

sbeaver-netgate Dec 28, 2016

Contributor

You have a version of pfSense that was produced in September of 2016. That version, like all versions in the 2.3 series had a bug that incorrectly rendered onchange events. Until you upgrade to a version newer than the one in which I fixed it, you will continue to see that bug.

The version of Squid you have installed is the latest version in which the Javascript now works correctly, so on page load it sees that Auth type = None and disables certain inputs.

Contributor

sbeaver-netgate replied Dec 28, 2016

You have a version of pfSense that was produced in September of 2016. That version, like all versions in the 2.3 series had a bug that incorrectly rendered onchange events. Until you upgrade to a version newer than the one in which I fixed it, you will continue to see that bug.

The version of Squid you have installed is the latest version in which the Javascript now works correctly, so on page load it sees that Auth type = None and disables certain inputs.

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta

lgcosta Dec 28, 2016

Contributor

thanks @rbgarga
@sbeaver-netgate and @doktornotor to what i know, 2.3.3 is a development release and it is not advisable to use it in a production environment. So i insisted on reporting this in the latest "stable" release.

Contributor

lgcosta replied Dec 28, 2016

thanks @rbgarga
@sbeaver-netgate and @doktornotor to what i know, 2.3.3 is a development release and it is not advisable to use it in a production environment. So i insisted on reporting this in the latest "stable" release.

@doktornotor

This comment has been minimized.

Show comment
Hide comment
@doktornotor

doktornotor Dec 28, 2016

Contributor

@rbgarga - I'd say the 2.3.3 release is rather overdue. The amount of fixes people are missing on "stable" branch gets annoying, not to mention the package versions gap where noone can see any of those fixes being done. It's not just Squid.

Contributor

doktornotor replied Dec 28, 2016

@rbgarga - I'd say the 2.3.3 release is rather overdue. The amount of fixes people are missing on "stable" branch gets annoying, not to mention the package versions gap where noone can see any of those fixes being done. It's not just Squid.

@LFCavalcanti

This comment has been minimized.

Show comment
Hide comment
@LFCavalcanti

LFCavalcanti Dec 28, 2016

@doktornotor , it is not pointless, there is an issue here... not the one we reported specifically, the issue is that 2.3.3 is being put on hold for too long and this was not merged into 2.3.2 "Stable". @rbgarga said he will reverse it, we will wait and test.

I don't see the need to be so blunt about this, remember not everyone here have the same vision of the overall development of the repo, so talking like you did can also be pointed out as pointless, no wonder people don't contribute as much, anyone trying to learn how things work get this treatment.

Let's be civil and technical to the point please.

LFCavalcanti replied Dec 28, 2016

@doktornotor , it is not pointless, there is an issue here... not the one we reported specifically, the issue is that 2.3.3 is being put on hold for too long and this was not merged into 2.3.2 "Stable". @rbgarga said he will reverse it, we will wait and test.

I don't see the need to be so blunt about this, remember not everyone here have the same vision of the overall development of the repo, so talking like you did can also be pointed out as pointless, no wonder people don't contribute as much, anyone trying to learn how things work get this treatment.

Let's be civil and technical to the point please.

@doktornotor

This comment has been minimized.

Show comment
Hide comment
@doktornotor

doktornotor Dec 28, 2016

Contributor

Sigh. Turning off GitHub email. Sorry guys.

Contributor

doktornotor replied Dec 28, 2016

Sigh. Turning off GitHub email. Sorry guys.

@sbeaver-netgate

This comment has been minimized.

Show comment
Hide comment
@sbeaver-netgate

sbeaver-netgate Dec 28, 2016

Contributor

Once the JS fixes in Squid 2.3.2 have been reverted, you should be able to uninstall/reinstall Squid and regain the ability to set all the fields. Obviously the automatic enable/disable of fields based on Auth type will not work though.

Contributor

sbeaver-netgate replied Dec 28, 2016

Once the JS fixes in Squid 2.3.2 have been reverted, you should be able to uninstall/reinstall Squid and regain the ability to set all the fields. Obviously the automatic enable/disable of fields based on Auth type will not work though.

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta
Contributor

lgcosta replied Dec 28, 2016

@LFCavalcanti

This comment has been minimized.

Show comment
Hide comment
@LFCavalcanti

LFCavalcanti Dec 28, 2016

@sbeaver-netgate , yep as it was before.

I tested here, restoring a backup with the auth config already set, it also fails, logging a parsing error on System Log.

Thanks!

We just didn't understand where this was commited and why the issue was happening in branch 2.3.2 as well. People need more patience for outsiders like me.

LFCavalcanti replied Dec 28, 2016

@sbeaver-netgate , yep as it was before.

I tested here, restoring a backup with the auth config already set, it also fails, logging a parsing error on System Log.

Thanks!

We just didn't understand where this was commited and why the issue was happening in branch 2.3.2 as well. People need more patience for outsiders like me.

@rbgarga

This comment has been minimized.

Show comment
Hide comment
@rbgarga

rbgarga Dec 28, 2016

Member

Done. 0.4.29_1 is going to show up soon

Member

rbgarga replied Dec 28, 2016

Done. 0.4.29_1 is going to show up soon

@sbeaver-netgate

This comment has been minimized.

Show comment
Hide comment
@sbeaver-netgate

sbeaver-netgate Dec 28, 2016

Contributor

Glad we got here :) The core dev team spend a LOT of time filtering bug reports that are really due to inexperience or lack of understanding. It does require patience and persistence to do this correctly, and we sometimes get it wrong. Sorry for that. Please don't let it prevent you from continuing to contribute. To have the opportunity to learn is a great privilege.

In this case your initial report focused on the incorrectly formatted "onchage" code and suggested we had introduced that issue, when in fact the reverse was true. Hence the confusion.

Contributor

sbeaver-netgate replied Dec 28, 2016

Glad we got here :) The core dev team spend a LOT of time filtering bug reports that are really due to inexperience or lack of understanding. It does require patience and persistence to do this correctly, and we sometimes get it wrong. Sorry for that. Please don't let it prevent you from continuing to contribute. To have the opportunity to learn is a great privilege.

In this case your initial report focused on the incorrectly formatted "onchage" code and suggested we had introduced that issue, when in fact the reverse was true. Hence the confusion.

@lgcosta

This comment has been minimized.

Show comment
Hide comment
@lgcosta

lgcosta Dec 28, 2016

Contributor

beautiful words @sbeaver-netgate thanks to the message !

Contributor

lgcosta replied Dec 28, 2016

beautiful words @sbeaver-netgate thanks to the message !

@atyltin

This comment has been minimized.

Show comment
Hide comment
@atyltin

atyltin Jan 11, 2017

Hi.
Maybe you know what happened in the new versions of squid?
2.3.2-RELEASE-p1 (amd64)
squid www 0.4.32
Squid Authentication WinBind
I have all times this same log in squid
Date IP Status Address User Assignment
10.01.2017 18:51:17 172.16.4.12 TCP_TUNNEL / 200 rs.mail.ru:443 admin 217.69.139.42
10.01.2017 18:51:12 172.16.4.12 TCP_DENIED / 407 rs.mail.ru:443 - -
10.01.2017 18:51:12 172.16.4.12 TCP_DENIED / 407 rs.mail.ru:443 - -
What is it? This is normal?
Thank you.

atyltin replied Jan 11, 2017

Hi.
Maybe you know what happened in the new versions of squid?
2.3.2-RELEASE-p1 (amd64)
squid www 0.4.32
Squid Authentication WinBind
I have all times this same log in squid
Date IP Status Address User Assignment
10.01.2017 18:51:17 172.16.4.12 TCP_TUNNEL / 200 rs.mail.ru:443 admin 217.69.139.42
10.01.2017 18:51:12 172.16.4.12 TCP_DENIED / 407 rs.mail.ru:443 - -
10.01.2017 18:51:12 172.16.4.12 TCP_DENIED / 407 rs.mail.ru:443 - -
What is it? This is normal?
Thank you.

@doktornotor

This comment has been minimized.

Show comment
Hide comment
@doktornotor

doktornotor Jan 11, 2017

Contributor

@atyltin

  • This ain't a support forum.
  • Your issue is absolutely off-topic and unrelated to the commit in here.
Contributor

doktornotor replied Jan 11, 2017

@atyltin

  • This ain't a support forum.
  • Your issue is absolutely off-topic and unrelated to the commit in here.
Please sign in to comment.