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

Upgrade Overall Experience #11

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

technowhizz
Copy link

@technowhizz technowhizz commented Oct 8, 2023

The changes are intended to improve the usability and functionality of the project.

Environment Configuration

  • Added env.php to bootstrap/app.php

Shortener Domains

  • Modified LinkHelper.php to add adf.ly to the list of shortener domains

Redirect Notice

  • Modified LinkController.php to enable a redirect notice if enabled in the environment

Link Ordering

  • Modified add_link_table_indexes.php to order links by ID when chunking them

CSS Styling

  • Modified CSS for site URL field and check button in index page

Input Validation

  • Added input validation script for link URL field in index page

Login Page Message

  • Updated login page with message from environment, if enabled

Admin Link

  • Added admin link to navbar dropdown menu on small screens

I'm not sure if this is the best way to achieve this, but it works.

Looking further into #9 I found that the useCurrent() function was returning 0000-00-00 00:00:00 instead of CURRENT_TIMESTAMP. The same with useCurrentOnUpdate(). This should hopefully avert this.

Open to other ideas as to why this might be happening but in the meantime this should work

(Messed up the previous PR hence opening this one)

@technowhizz
Copy link
Author

From #10

@technowhizz This patch looks great; thank you for your efforts. I have one question about database/migrations/2023_01_24_234401_add_secrets_key_default.php. The code in my repository uses Schema modification commands to remain DB agnostic. I would like to allow for other DBs to be used with POLR in the future. The update I'm seeing is an ALTER statement that only works for MySQL. I don't have much experience with Laravel; this upgrade was a knee-jerk to get to PHP 8. Do you see any technical reason that we could not keep the declarative syntax using Schema?

@technowhizz
Copy link
Author

@mwilmes-at-avc I too would love to keep the laravel code to interact with the DB but for some reason it just didnt seem to work. I think others have also faced this issue as noted by @nitz in #9

The issue is also noted at laravel/framework#3602

However it seems like telling laravel that you have a strict db in database.php may fix it. However, this is the default in the repo and hasnt fixed it for me.

Restores the V2 Api as it was in previous versions of Polr and moves the
new Api to V3. This is so that there are no breaking changes in the API.
Additionally adds a delete endpoint to the V2 Api.
Adds back the api middleware to the api route.
Removes a number of URLs from the list of domains that can be shortened.
Adds a JS help meesaged that is shown if the link is invalid or missing
the protocol (http:://,https://)
Changes the colour of the shortened site address when clicking link
option. Also fixed spacing of the check availability button.
This adds the admin page to the menu for all users. However the page
only loads for admin users.
Adds the ability to specify a message in the .env `RDR_MESSAGE` which
will be displayed for `RDR_DELAY_TIME` seconds if
`ENEABLE_RDR_NOTICE` is true else it wont be displated.
Adds the ability to show a message on that login screen that can be
defined in the .env. The message text variable is `LOGIN_MESSAGE` and it
will be shown if `ENABLE_LOGIN_MESSAGE` is true
Change the 301 (permanent) redirect to 302 (temporary) redirect in
LinkController.php. This is because the 301 redirect is cached by
browsers which leads to issues when link are updated/deleted. The links
should be 302 as the long link can change and nowadays the overhead of
a 302 redirect is minimal.
@technowhizz
Copy link
Author

@mwilmes-at-avc After searching around quite a bit, the code should work but for some reason it just doesn't. As far as I can tell the only way to fix this for now is with the raw SQL. I understand if that means that you no longer want to merge this in as it doesn't permit the use of postgres.

If anyone does manage to fix it do let me know

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.

1 participant