Skip to content

Conversation

tristanbes
Copy link
Contributor

No description provided.

… displayed because of the rendered path with DIR. I tryed to make it as clean as possible
@weaverryan
Copy link
Member

I think I see what you're getting at, but this isn't quite right. The getFullPath() method must be the absolute file path to the file, as it's used to unlink the file.

Perhaps 3 methods, something like getWebPath (e.g. /uploads/photos/foo.jpg), getAbsolutePath (e.g. /path/to/app/web/uploads/photos/foo.jpg) and getUploadDir (e.g. /path/to/app/web/uploads/photos)?

@tristanbes
Copy link
Contributor Author

yeah that sounds right to me. Sry for the mistake :)

@tristanbes
Copy link
Contributor Author

@weaverryan something like that ? :)

{
return null === $this->path ? null : $this->getUploadRootDir().'/'.$this->path;
}

public function getWebPath()
{
return null === $this->path ? null : $this->getUploadDir().'/'.$this->path;
Copy link
Member

Choose a reason for hiding this comment

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

calling it recursively will lead to an infinite loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry but i don't see where there is a possible infinite loop :o. Maybe it was in the previous commit. I fixed it :p

Copy link
Member

Choose a reason for hiding this comment

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

I loaded the page before the commit fixing it. Github does not refresh the page automatically when you add a commit to a pull request :)

Copy link
Member

Choose a reason for hiding this comment

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

Do you think that the getWebPath() should return a path that starts with a slash or not?

uploads/documents/foo.pdf

versus

/uploads/documents/foo.pdf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stof thinks that is should not start with a leading slash for portability ( see comments here on my closed pull request #557 )

Copy link
Member

Choose a reason for hiding this comment

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

no slash as you would have to remove it to use it in asset(). A path starting with a slash in asset() is not modified as it s assumed to be relative to the root of the domain. A path that don't start by a slash is assumed to be relative to the web folder (This concern the case where you don't configure the helper to use a CDN of course)

Copy link
Member

Choose a reason for hiding this comment

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

@stof Yes sir, you're right - I've been omitting the leading slash without realizing it.

weaverryan added a commit that referenced this pull request Jul 27, 2011
Changed the name of the method to be more revelent. Fix the leading slash on the getWebLocation() for portability
@weaverryan weaverryan merged commit a97bf67 into symfony:master Jul 27, 2011
@weaverryan
Copy link
Member

Merged - thanks for everyone's help! I did make a few small changes - there were two method references I noticed that we missed. If you spot anything else, let me know

bce50c0

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.

4 participants