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

Update theme.inc to fix the void error. #171

Closed
wants to merge 1 commit into from
Closed

Conversation

simonhm
Copy link

@simonhm simonhm commented Sep 20, 2022

What does this Pull Request do?

Fixing the Void Error when viewing a newspaper collection

What's new?

How should this be tested?

  • Clicking on "Expand all months" link currently opens a void site with an error. The URL that gets listed is https://void%280%29/.

2022-09-20

  • Apply this patch
  • Try to click on "Expand all months" again. It should work as expected

Additional Notes:

Here is only my quick suggested solution. Feel free to have better solution for this issue. For some reasons, Drupal is removing "javascript:" from the href attribute of the "Expand all months" link.

Example:

  • Does this change the interface, add a new feature, or otherwise change behaviours that would require updating documentation? No
  • Does this change add any new dependencies? No
  • Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)? No
  • Could this change impact execution of existing code? No

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/7-x-1-x-committers

@islandora-community
Copy link

@Islandora/legacy-committers

@wgilling
Copy link

In order to test this on an Ubuntu machine, https://github.com/Islandora-Labs/islandora_vagrant | and run "islandora vagrant" - then SSH into the container to check out the branch should allow me to test this.

@MorganDawe
Copy link
Contributor

Came across this issue, which was fixed in the D.O issue 3310081. We could take advantage of this fix by simply changing the void function call to a properly formatted javascript:void(0) instead, since Drupal core is considering this approach 'harmless' javascript path. Tested this approach on a local box and seams to work just fine in 7.94.

Using the id selector approach instead here is not that bad, since javascript:void(0) could potentially violate Content Security Policy on CSP-enabled HTTPS pages.

Alternatively, one could also use drupal_html_id(), assign the id to the links render array, and use it instead of the static id (ex: '#collapse'), however, using an href as an #id will result in the URL being modified when clicking expand/collapse, to include the id. If the id however does not appear in the DOM, feels somewhat misleading.

Kinda leaning towards the javascript:void(0) approach, my two cents.

@adam-vessey
Copy link
Contributor

@simonhm: I suspect that this has instead been addressed via #172, and as such, this PR can probably be closed?

@adam-vessey
Copy link
Contributor

I'm going to go ahead and close this for now. If this is indeed still an issue, feel free to reopen the PR.

@adam-vessey adam-vessey closed this Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants